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

Only compile non-module exports for export-dep-as-jar goal #8914

Conversation

hrfuller
Copy link
Member

@hrfuller hrfuller commented Jan 7, 2020

Problem

When running the export-dep-as-jar Goal we compile all targets in the build context. This is unnecessary and can be problematic if any of the target roots have compile errors in them, because the export will fail. It is unnecessary because the target roots are being exported as source modules #8812

Solution

We exclude target_roots, and their dependees from the compile context.

Result

Only targets in the build context that are not being exported as sources are compiled.

@hrfuller hrfuller marked this pull request as ready for review January 7, 2020 21:57
@hrfuller
Copy link
Member Author

hrfuller commented Jan 7, 2020

@wisechengyi @blorente @stuhood (maybe) reviews please :)

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

This looks awesome! I'm looking forward to try this out.

Would it make sense to expose a flag in export-classpath that uses the new export_dep_as_jar_classpath product?

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Does this change affect how resources are packaged? Looking at the implementation I assume that resources in target roots continue to be packaged in jars like before but just want to make sure.

@wisechengyi
Copy link
Contributor

wisechengyi commented Jan 7, 2020

@olafurpg

Would it make sense to expose a flag in export-classpath that uses the new export_dep_as_jar_classpath product?

this is not technically feasible from v1 right now, because product required initialized before options and cannot be changed by options. However, we could still make a separate task for it.

@stuhood
Copy link
Member

stuhood commented Jan 8, 2020

As a general comment, the export-deps-as-jars and export-classpath split is a bit awkward. We should be looking for opportunities to unify them in the future.

this is not technically feasible from v1 right now, because product required initialized before options and cannot be changed by options. However, we could still make a separate task for it.

@wisechengyi : This isn't accurate I don't think? The prepare method receives options to decide what to declare a dependency on.

Copy link
Member Author

@hrfuller hrfuller left a comment

Choose a reason for hiding this comment

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

.

@hrfuller
Copy link
Member Author

hrfuller commented Jan 8, 2020

Hey @olafurpg re:

Does this change affect how resources are packaged? Looking at the implementation I assume that resources in target roots continue to be packaged in jars like before but just want to make sure.

This PR doesn't change how resources in target roots are packaged. Only whether the sources are compiled or not.

Though looking at https://github.com/pantsbuild/pants/pull/8812/files#diff-fc0db43987b87bb51793c258ab96b407R347 from #8812 it seems like non-modulizable targets (which include target roots,) may not zip resources into jars because this line https://github.com/pantsbuild/pants/pull/8812/files#diff-fc0db43987b87bb51793c258ab96b407R301 is now in a separate function which isn't applied to all targets.

Copy link
Contributor

@wisechengyi wisechengyi left a comment

Choose a reason for hiding this comment

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

Thanks, Henry! Looks mostly good.

@hrfuller hrfuller requested a review from wisechengyi January 9, 2020 22:32
@stuhood stuhood changed the title Only compile non-module exports Only compile non-module exports for export-dep-as-jar goal Jan 10, 2020
Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

One thing to watch out for, but no blockers. Thanks!

# so however we compute runtime_classpath it will contain all the nescessary jars for
# the export_dep_as_jar_classpath.export_dep_as_jar_classpath.
if self.context.products.is_required_data('export_dep_as_jar_classpath'):
self.context.products.get_data('export_dep_as_jar_classpath', classpath_product.copy)
Copy link
Member

Choose a reason for hiding this comment

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

It's fine for now, but get_data isn't guaranteed to have the sideeffect of calling the initializer: if you ever installed another task to compute additional entries for export_dep_as_jar_classpath and it ran first, this would break, because copy would not be called.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good to know for the future, thanks!

@wisechengyi wisechengyi merged commit 8546623 into pantsbuild:master Jan 10, 2020
@wisechengyi wisechengyi deleted the hfuller/partial-compile-for-export-dep-as-jar branch January 10, 2020 19:22
wisechengyi pushed a commit that referenced this pull request Jan 21, 2020
### Problem

When running the `export-dep-as-jar` Goal we compile all targets in the build context. This is unnecessary and can be problematic if any of the target roots have compile errors in them, because the export will fail. It is unnecessary because the target roots are being exported as source modules #8812 

### Solution

We exclude target_roots, and their dependees from the compile context.

### Result

Only targets in the build context that are not being exported as sources are compiled.
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.

4 participants