Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alan/osgi #241

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Alan/osgi #241

wants to merge 7 commits into from

Conversation

easye
Copy link
Collaborator

@easye easye commented Jun 26, 2020

Re-apply @alanruttenberg's work on OSGI #135 to abcl-1.7.1-dev

In local testing these changes bork basic usage of abcl-asdf:resolve for Maven components not in bundles. Creating a merge request to trigger the CI build for feedback to Alan.

Copy link
Collaborator Author

@easye easye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alanruttenberg

I rebased your changes onto abcl-1.7.1-dev as #241 for which you can see the failure under the CI as https://travis-ci.org/github/armedbear/abcl/jobs/702274183#L8225.

Running these changes locally, I was not able to do a simple abcl-asdf:resolve for dependencies.

Please add a test or two for the OSGI environment (or give me instructions) so I can ensure that the OSGI loading continues to work in the future.

@@ -0,0 +1,25 @@
(in-package :asdf)

(defclass bundle (jar-file)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ASDF has a bundle-op, so to avoid confusion please rename to osgi-bundle.

@@ -428,6 +428,7 @@ hint."

(defun make-session (repository-system)
"Construct a new aether.RepositorySystemSession from the specified REPOSITORY-SYSTEM."
(unless *init* (init))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove. the with-aether macro now does the necessary initialization.

(if (consp resolved)
(jcall "loadClass" (car resolved) (second resolved))
(jclass resolved))))
))))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style: please no dangling parenthesis.

(invoke-restargs ,name ,object-var ,args-var ,(eql arg 0)))))))
(defun read-invoke (stream char arg)
(if (eql arg 1)
(progn (require 'javaparser)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. javaparser is an ASDF system so please use asdf:load-system

  2. Thunking through the load routines each time read-invoke is gonna be expensive isn't it? Maybe just move the javaparser into a dependent system of jss as jss/javaparser always loading this?

@@ -304,6 +331,15 @@ want to avoid the overhead of the dynamic dispatch."
(ambiguous matches))))
(ambiguous matches))))))))))

;; Interactive use: Give a full class name as a string, return the shortest unique abbreviation
(defun shortest-unambiguous-java-class-abbreviation(name &optional as-string?)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool. Always wanted to code this one…

@@ -512,28 +545,33 @@ associated is used to look up the static FIELD."
(when *do-auto-imports*
(do-auto-imports)))

(defun japropos (string)
(defun japropos (string &optional (fn (lambda(match type bundle?) (format t "~a: ~a~a~%" match type bundle?))))
Copy link
Collaborator Author

@easye easye Jun 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda convoluted.

  1. Rewrite as a keyword argument, &optional args are hard to refactor when we need further changes.

  2. Please document better.

@@ -1,6 +1,10 @@
(eval-when (:compile-toplevel :load-toplevel :execute)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work.

Please fix, moving out of cl-user.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants