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.

Sign up for GitHub

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

Jump to bottom

Refactor BuildChainBuilder #36696

Merged
merged 1 commit into from
Oct 26, 2023
Merged

Refactor BuildChainBuilder #36696

merged 1 commit into from
Oct 26, 2023

Conversation

mariofusco
Copy link
Contributor

@mariofusco mariofusco commented Oct 25, 2023 •
edited

I'm studying how the BuildChainBuilder works in order to document it and while doing so I found some inefficiency and dead code. I also refactored it a bit hoping to improve readability.

/cc @dmlloyd @franz1981 @geoand @maxandersen

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 25, 2023

Thanks for your pull request!

The title of your pull request does not follow our editorial rules. Could you have a look?

  • title should preferably start with an uppercase character (if it makes sense!)

This message is automatically generated by a bot.

@mariofusco mariofusco changed the title refactor BuildChainBuilder Refactor BuildChainBuilder Oct 25, 2023
dmlloyd
dmlloyd approved these changes Oct 25, 2023
View reviewed changes
Copy link
Member

@dmlloyd dmlloyd left a comment

Choose a reason for hiding this comment

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

LGTM

@dmlloyd
Copy link
Member

dmlloyd commented Oct 25, 2023

Thanks for looking at this. These are good cleanups; I was a bit rushed when it was first written ;)

franz1981
franz1981 reviewed Oct 25, 2023
View reviewed changes
core/builder/src/main/java/io/quarkus/builder/BuildExecutionBuilder.java
@@ -24,8 +24,8 @@ public final class BuildExecutionBuilder {
BuildExecutionBuilder(final BuildChain buildChain, final String buildTargetName) {
this.buildChain = buildChain;
this.buildTargetName = buildTargetName;
initialSingle = new HashMap<>(buildChain.getInitialSingleCount());
initialMulti = new HashMap<>(buildChain.getInitialMultiCount());
initialSingle = new HashMap<>();
Copy link
Contributor

@franz1981 franz1981 Oct 25, 2023 •
edited

Choose a reason for hiding this comment

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

Just a question, why not use the known capacity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those 2 variable were initialized to 0 here, never touched again during the graph linearization algorithm, inserted in the buildChain result (still always with value 0) here and then read to initialize the size of the maps were you pointed. In essence those maps were always initialized with capacity 0.

@quarkus-bot
Copy link

quarkus-bot bot commented Oct 26, 2023

Failing Jobs - Building 2823db7

Status Name Step Failures Logs Raw logs Build scan
✔️ JVM Tests - JDK 11
✔️ JVM Tests - JDK 17
✖ JVM Tests - JDK 17 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 21
✔️ Maven Tests - JDK 11
✖ Maven Tests - JDK 11 Windows Build Failures Logs Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 17 Windows #

- Failing: extensions/vertx-http/deployment 
! Skipped: devtools/cli extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 364 more

📦 extensions/vertx-http/deployment

✖ io.quarkus.vertx.http.proxy.TrustedForwarderProxyFailureTest.testHeadersAreIgnored line 29 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:288)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:314)

✖ io.quarkus.vertx.http.proxy.TrustedXForwarderProxiesUnknownHostnameFailureTest.testHeadersAreIgnored line 17 - More details - Source on GitHub

java.net.SocketTimeoutException: Read timed out
	at java.base/sun.nio.ch.NioSocketImpl.timedRead(NioSocketImpl.java:288)
	at java.base/sun.nio.ch.NioSocketImpl.implRead(NioSocketImpl.java:314)

⚙️ Maven Tests - JDK 11 Windows #

📦 integration-tests/maven

✖ io.quarkus.maven.it.DevMojoIT.testExternalReloadableArtifacts line 1448 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.DevMojoIT was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.DevMojoIT.testExternalReloadableArtifacts line 1448 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.DevMojoIT was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.JarRunnerIT.testNonAsciiDir line 70 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

✖ io.quarkus.maven.it.JarRunnerIT.testNonAsciiDir line 70 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

✖ io.quarkus.maven.it.JarRunnerIT.testPlatformPropertiesOverridenInApplicationProperties line 135 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.JarRunnerIT that uses io.quarkus.maven.it.verifier.MavenProcessInvocationResult was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.JarRunnerIT.testPlatformPropertiesOverridenInApplicationProperties line 135 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.JarRunnerIT that uses io.quarkus.maven.it.verifier.MavenProcessInvocationResult was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.UberJarQuarkusIntegrationTestIT.testUberJar line 13 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.QuarkusITBase that uses io.quarkus.maven.it.verifier.MavenProcessInvocationResult was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

✖ io.quarkus.maven.it.UberJarQuarkusIntegrationTestIT.testUberJar line 13 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in io.quarkus.maven.it.QuarkusITBase that uses io.quarkus.maven.it.verifier.MavenProcessInvocationResult was not fulfilled within 1 minutes.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

@geoand geoand merged commit 4645e2a into quarkusio:main Oct 26, 2023
49 of 51 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.6 - main milestone Oct 26, 2023
@mariofusco mariofusco deleted the build_chain_refactor branch October 26, 2023 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@franz1981 franz1981 franz1981 left review comments

@dmlloyd dmlloyd dmlloyd approved these changes

@geoand geoand geoand approved these changes

Assignees
No one assigned
Labels
area/core
Projects
None yet
Milestone
3.6.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mariofusco @dmlloyd @geoand @franz1981