-
Notifications
You must be signed in to change notification settings - Fork 212
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
Composite builds support for nested jars #992
base: exp/1.5
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont fully understand the problem tbh, this doesnt seem like the correct solution at all to me. A Configuration
is a FileCollection
so you should be able to do .from() on it getNestedJars()
.
An intergration test is needed for this as well, it might help me understand what the problem is.
@Deprecated | ||
@InputFiles | ||
public abstract ConfigurableFileCollection getNestedJars(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot deprecate this, as its used for adding files that arent part of a Configuration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding to this collection directly doesn't add any metadata even if the file isn't actually a mod jar. Files can also be added directly to Configuration
s. So it doesn't really make sense to me to actually keep this.
@InputFiles | ||
public FileCollection getNestedDependenciesResolved() { // for task up-to-date checking | ||
final ConfigurableFileCollection files = getProject().files(); | ||
|
||
if (getAddNestedDependencies().get()) { | ||
getNestedConfigurations().get().forEach(files::from); | ||
files.from(getNestedJars()); | ||
} | ||
|
||
return files; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this any different from putting the Configuration
in the existing ConfigurableFileCollection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding configuration directly to the existing ConfigurableFileCollection
causes duplicates. That collection also doesn't take care of metadata.
Based on `simple` test
Thanks for adding a test, im still not super happy with this fix, but dont have a better suggestion off the top of my head. Ill have to sit down and take a better look into this. |
This is likely superseded by #1080, which is a more idiomatic approach - storing |
Resolves #975
This delays dependency resolution for nested jars until it is actually needed because dependencies substituted from included builds doesn't actually resolve during configuration phase.
This also adds
nestedConfigurations
forRemapJar
, allowing custom configurations to be used for inclusion.Note that this still retains the behavior of not adding transitive dependencies as nested jars.