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

Set the CLASSPATH for repl targets #1572

Merged
merged 7 commits into from
Jul 28, 2021
Merged

Set the CLASSPATH for repl targets #1572

merged 7 commits into from
Jul 28, 2021

Conversation

facundominguez
Copy link
Member

No description provided.

@facundominguez
Copy link
Member Author

cc @buonuomo

haskell/repl.bzl Outdated Show resolved Hide resolved
@facundominguez
Copy link
Member Author

If you have any suggestions on how to add a test involving the repl, I could give it a try.

@facundominguez
Copy link
Member Author

If you have any suggestions on how to add a test involving the repl, I could give it a try.

hm, I think I could try something similar to:
https://github.com/tweag/rules_haskell/blob/master/tests/RunTests.hs#L67-L69

haskell/private/haskell_impl.bzl Outdated Show resolved Hide resolved
haskell/repl.bzl Outdated Show resolved Hide resolved
haskell/repl.bzl Outdated Show resolved Hide resolved
haskell/repl.bzl Outdated
@@ -74,6 +76,7 @@ HaskellReplCollectInfo = provider(
fields = {
"load_infos": "Dictionary from labels to HaskellReplLoadInfo.",
"dep_infos": "Dictionary from labels to HaskellReplDepInfo.",
"java_deps": "depset of Files to jars needed for building.",
Copy link
Member

Choose a reason for hiding this comment

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

Could this be carried in HaskellReplLoadInfo instead?

The idea here is that HaskellReplCollectInfo collects "load from source" and "load from binary" information separately for each target associated to the target's label. This information is then combined into HaskellReplInfo later on based on the from_source and from_binary patterns. Adding java_deps directly here changes that design. Is there a good reason to do so?


If I understand correctly then JARs are only compile time dependencies for targets loaded from source to interpret inline-java TH splices. With targets loaded from binary they are only runtime dependencies and so should only be made available if collect_data = True to be consistent. Does that make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

If I understand correctly then JARs are only compile time dependencies for targets loaded from source to interpret inline-java TH splices.

JARs are both compile and runtime dependencies in this case.

With targets loaded from binary they are only runtime dependencies and so should only be made available if collect_data = True to be consistent.

This makes sense.

Copy link
Member Author

@facundominguez facundominguez Jul 28, 2021

Choose a reason for hiding this comment

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

I still don't understand the proposed rearrangement.

If I move java_deps to HaskellReplLoadInfo, how do I keep track of the jars that need to be added to the classpath when the target is loaded from binary?

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, strictly speaking, the CLASSPATH doesn't need to be set when running. But it would be a convenient way to signal to the program the list of jar paths to feed to the JVM when initializing it.

This issue is related. Maybe we should handle this concern separately.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved java_deps to HaskellReplLoadInfo.

Copy link
Member

Choose a reason for hiding this comment

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

If I move java_deps to HaskellReplLoadInfo, how do I keep track of the jars that need to be added to the classpath when the target is loaded from binary?

Right, so they would also be needed in HaskellReplDepInfo for the runtime deps case.

Copy link
Member

Choose a reason for hiding this comment

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

hm, strictly speaking, the CLASSPATH doesn't need to be set when running. But it would be a convenient way to signal to the program the list of jar paths to feed to the JVM when initializing it.

This issue is related. Maybe we should handle this concern separately.

Oh, I remember. Yes, happy to treat this separately.

haskell/private/haskell_impl.bzl Outdated Show resolved Hide resolved
Copy link
Member

@aherrmann aherrmann left a comment

Choose a reason for hiding this comment

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

Thank you, looks good. Thank you for adding the test case!

@aherrmann aherrmann added the merge-queue merge on green CI label Jul 28, 2021
@mergify mergify bot merged commit d162b81 into master Jul 28, 2021
@mergify mergify bot deleted the fd/classpath-repl branch July 28, 2021 14:12
@mergify mergify bot removed the merge-queue merge on green CI label Jul 28, 2021
@facundominguez
Copy link
Member Author

Thanks for reviewing!

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