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

Build Succeeds, when Reusing an Artifact with Enabled Compression #41930

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

gcw-it
Copy link
Contributor

@gcw-it gcw-it commented Jul 16, 2024

Fix #41373

The NativeImageBuildItem tracks now, if the executable was created or reused.
The UpxCompressionBuildStep skips compression, when the executable is reused.

Any change to the compression settings between builds, while reusing an existing artifact, will have no effect.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks good and it's a nice fix. Thanks!

I added a very minor comment. Let me know if you want to address it or not.

if (nativeConfig.compression().level().isEmpty()) {
log.debug("UPX compression disabled");
return;
}
if (image.isReused()) {
log.info("Native executable reused: skipping compression");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure if we want an info line here. I would demote it to debug given it's really an implementation detail (I mean you for sure don't expect things to be compressed twice if you reuse an executable).

The why is that we are trying hard to avoid having a lot of log lines so that people don't miss anything important.

Do you agree? (Don't hesitate to push back if you don't) If so, either push and squash or I'll be happy to do it if you are not comfortable with git yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This sounds good to me.
Done.

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Let's wait for CI to complete and merge.

Thanks for handling this one!

@gsmet gsmet added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Jul 17, 2024
Copy link

quarkus-bot bot commented Jul 17, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit d653599.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17 Windows

📦 integration-tests/grpc-hibernate

com.example.grpc.hibernate.BlockingRawTest.shouldAdd - History

  • Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in com.example.grpc.hibernate.BlockingRawTestBase was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at com.example.grpc.hibernate.BlockingRawTestBase.shouldAdd(BlockingRawTestBase.java:59)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

@gsmet gsmet merged commit a5c779e into quarkusio:main Jul 17, 2024
52 checks passed
@quarkus-bot quarkus-bot bot added kind/bugfix and removed triage/waiting-for-ci Ready to merge when CI successfully finishes labels Jul 17, 2024
@quarkus-bot quarkus-bot bot added this to the 3.14 - main milestone Jul 17, 2024
@gsmet
Copy link
Member

gsmet commented Jul 17, 2024

Merged, thanks a lot!

@galderz
Copy link
Member

galderz commented Jul 23, 2024

Kudos @gcw-it for getting this to work!

@gcw-it gcw-it deleted the pr_41737 branch July 24, 2024 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Native Build Fails, when Reusing Existing Executable with Compression Enabled
3 participants