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

Always build the bin/pulumi-java-gen target #737

Closed
wants to merge 1 commit into from

Conversation

guineveresaenger
Copy link
Contributor

Running make build_sdks locally often fails on java-gen if there is an old java gen binary left in the (not source controlled) provider/bin folder.
Making the binary build a phony target ensures we re-build it every time an pull the java gen binary of the version specified in the provider config and Makefile.

@guineveresaenger guineveresaenger requested review from t0yv0 and a team December 6, 2023 21:36
Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

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

This feels like the fix is taking us in the wrong direction - we want to introduce more file-dependencies and less phonies as this allows for more efficient builds and therefore more responsive development experience.

The ideal fix here would be to address the missing dependency for bin/pulumi-java-gen – the version. So, when the version changes, we want to re-download the binary. This can be done by moving the java version into a file (e.g. java-gen.version) which is then depended on for the bin/pulumi-java-gen target, then the content of that file is read and passed to the pulumictl download-binary command. Then the process of upgrading java is writing a new version to that file.

@guineveresaenger
Copy link
Contributor Author

this allows for more efficient builds and therefore more responsive development experience

Can you talk about this a bit more? For this to have any impact, wouldn't we have to change a lot more of the phony targets? Is there a plan for this? By how much, specifically, would adding this change as-is worsen the development experience?

@iwahbe
Copy link
Member

iwahbe commented Dec 7, 2023

We had code that did this by inspecting the downloaded binary directly in pulumi-aws. It was introduced here, but removed when we normalized pulumi-aws.

An even simpler solution might be to postfix the java path by its version. Instead of downloading to a binary called bin/pulumi-java-gen, download to a binary called bin/pulumi-java-gen-$(JAVA_GEN_VERSION).

@thomas11
Copy link
Contributor

thomas11 commented Dec 7, 2023

We had code that did this by inspecting the downloaded binary directly in pulumi-aws. It was introduced here, but removed when we normalized pulumi-aws.

An even simpler solution might be to postfix the java path by its version. Instead of downloading to a binary called bin/pulumi-java-gen, download to a binary called bin/pulumi-java-gen-$(JAVA_GEN_VERSION).

Azure Native uses a sentinel file .pulumi-java-gen.version containing the version. It's used in this Make target:

bin/pulumi-java-gen: .pulumi-java-gen.version bin/pulumictl
	@mkdir -p bin
	bin/pulumictl download-binary -n pulumi-language-java -v $(shell cat .pulumi-java-gen.version) -r pulumi/pulumi-java

Make considers the target out of date and re-downloads pulumi-java only if the timestamp of the sentinel file is newer, i.e., someone edited it to bump the version.

@danielrbradley
Copy link
Member

Can you talk about this a bit more?

Sure thing! As part of our quality and local development experience improvements we want to continue to iterate on our makefiles to model dependencies as accurately as possible. @VenelinMartinov is also looking at this - to get to the point where the whole provider build can be marked as up-to-date.

The starting point of these changes is by fixing up the targets at the bottom of the dependency tree on which everthing else depends because, if these are marked as phony, then anything that depends on them will also never be able to be up to date.

An even simpler solution might be to postfix the java path by its version. Instead of downloading to a binary called bin/pulumi-java-gen, download to a binary called bin/pulumi-java-gen-$(JAVA_GEN_VERSION).

This is an interesting idea - I'd be happy with either approach - writing the version to a separate file, or putting the version in the filename would both work.

Putting the version into the filename of the binary saves having another file in the repository, but I'm not sure if changing the name of the binary is something that pulumictl download-binary supports yet. The other slight downside is that it's slightly more messy code to always reference bin/pulumi-java-gen-$(JAVA_GEN_VERSION) rather than just bin/pulumi-java-gen, but that's a minor nit pick.

I've just noticed that we've already got the "version in a file" approach used for downloading the pulumi CLI which depends on .pulumi/version. So, to follow that pattern I'd probably lean towards putting the version number into a file at .pulumi/java-gen-version.

@guineveresaenger
Copy link
Contributor Author

Closing as this is handled via #506 and #747 .

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