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

feat: add support for using custom .m2 directory #3573

Merged
merged 9 commits into from
Jun 13, 2022
Merged

Conversation

agdimech
Copy link
Contributor

@agdimech agdimech commented Jun 4, 2022

The existing Java packaging process uses a localRepository which points to a newly created tmp directory. This means that when building, all dependencies need to be freshly downloaded which is contributing a significant portion of time to the packaging process.

This PR provides the option to the user (hidden) to use a non-transient .m2 local repository if they so choose. I have experienced a huge improvement to performance by enabling this setting (5s with enabled vs 4 minutes using defaults).


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

.option('java-use-default-m2', {
type: 'boolean',
desc: 'Use default M2 repository when packaging a Java package.',
default: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have set the default to false for this setting to maintain backward compatibility. I am wandering whether this should be defaulted to true in future as I am not sure what negative side effects there would be by using the user's .m2 repository.

@RomainMuller
Copy link
Contributor

We moved away from the default cache locations for both .NET and Java because this had surprising consequences due to how both will always prefer artifacts already in cache (which is good), AND store the outputs of local builds into this cache (which is where surprises come from).

I reckon the performance improvement you see comes from not re-downloading dependencies on each build... which is also achieved when using a non-default, but also non-transient cache. Maybe the right think is to allow configuring a specific .m2 directory (which could be the default one, if that is what the user wants). This would be more flexible and also feels safer...

@RomainMuller RomainMuller changed the title feat: add support for using default .m2 when packaging Java feat: add support for using custom .m2 directory Jun 8, 2022
packages/jsii-pacmak/bin/jsii-pacmak.ts Outdated Show resolved Hide resolved
packages/jsii-pacmak/lib/targets/java.ts Outdated Show resolved Hide resolved
@RomainMuller
Copy link
Contributor

Can you perhaps also see to use that configuration flag somewhere (e.g: in one of the flavors of the build test, perhaps)?

RomainMuller
RomainMuller previously approved these changes Jun 13, 2022
@mergify mergify bot dismissed RomainMuller’s stale review June 13, 2022 13:45

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

Thank you for contributing! ❤️ I will now look into making sure the PR is up-to-date, then proceed to try and merge it!

@mergify mergify bot added the pr/ready-to-merge This PR is ready to be merged. label Jun 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

Merging (with squash)...

1 similar comment
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

Merging (with squash)...

@mergify mergify bot merged commit 0003c8a into aws:main Jun 13, 2022
@mergify
Copy link
Contributor

mergify bot commented Jun 13, 2022

Merging (with squash)...

@mergify mergify bot removed the pr/ready-to-merge This PR is ready to be merged. label Jun 13, 2022
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