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

single-artifact fix for AOT bug #218

Merged
merged 2 commits into from
May 22, 2015
Merged

single-artifact fix for AOT bug #218

merged 2 commits into from
May 22, 2015

Conversation

martinraison
Copy link
Contributor

Here it is. This should fix plumatic/plumbing#74. I ran the tests against Clojure 1.6.0 and 1.7.0-RC1, everything passes in both cases.

@w01fe
Copy link
Member

w01fe commented May 22, 2015

image

Thank you!!

My one concern is that emit-impl is private, and as such could cease to exist or change arbitrarily at any point in the future, which would create a new compatibility issue.

Two potential solutions that ensure forward compatibility:

Thoughts?

Thanks again, much appreciated!

@martinraison
Copy link
Contributor Author

Thanks for your feedback!

I updated the request with a more robust check. I basically try to call extend-protocol with fn excluded, and revert the exclusion if an exception is thrown. Is that close to what you had in mind?

I usually prefer avoiding direct version checks, especially since in this case we would need to check the version qualifier as well (1.7.0-RC1 is fixed, 1.7.0-alpha1 is not). This may not play well with forks of the Clojure compiler that use different version names. The only drawback I can think of is the unused "CLJ1195Test" interface lying around. I don't think this will impact anyone, though.

@w01fe
Copy link
Member

w01fe commented May 22, 2015

Agree a functionality check is better than a version check, and this looks like a safe way to do it. Thanks again for your contribution, much appreciated!

Did you also test with AOT to confirm that the root issue is fixed? Either way, I'll merge now, cut a snapshot and ask for feedback on the plumbing thread, and then cut a real release as soon as we have confirmation that everything looks good.

w01fe added a commit that referenced this pull request May 22, 2015
single-artifact fix for AOT bug
@w01fe w01fe merged commit 2b4534a into plumatic:master May 22, 2015
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.

uberjar-ed program using plumbing library above 0.3.4 throws exception
2 participants