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

Initialize build runner in AwtProcessor before usage #34296

Conversation

turing85
Copy link
Contributor

No description provided.

@quarkus-bot
Copy link

quarkus-bot bot commented Jun 26, 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 not end up with dot

This message is automatically generated by a bot.

@turing85 turing85 changed the title Make the GraalVM version command quiet. Make the GraalVM version command quiet Jun 26, 2023
@maxandersen
Copy link
Member

can you give some context of why you need it quiet?

@turing85
Copy link
Contributor Author

See this zulip discussion for details.

The issue was already fixed in this PR.

@turing85 turing85 force-pushed the feature/quiet-docker-command-for-determining-java-version branch from b0c6ac7 to 3614e84 Compare June 26, 2023 15:26
@turing85 turing85 force-pushed the feature/quiet-docker-command-for-determining-java-version branch 2 times, most recently from 797444d to f996e16 Compare June 26, 2023 15:32
@turing85 turing85 changed the title Make the GraalVM version command quiet Initialize build runner, make image pull- and GraalVM-version container commands quiet Jun 26, 2023
Comment on lines +104 to +105
nativeImageRunnerBuildItem.getBuildRunner()
.setup(processInheritIODisabled.isPresent() || processInheritIODisabledBuildItem.isPresent());
Copy link
Member

Choose a reason for hiding this comment

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

If we have to do that, there is something wrong. I'll have to have a look later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe I am misusing the setup(...) method, but I can say that setup() has not yet been called when AwtProcessor is executed. Without the call to setup(...), the successive call to nativeImageRunnerBuildItem.getBuildRunner().getGraalVMVersion() would trigger the initial image pull.

@turing85
Copy link
Contributor Author

turing85 commented Jun 27, 2023

Seems that the docker version used in github-workflows has some kind of bug:

[INFO] [io.quarkus.deployment.pkg.steps.NativeImageBuildRunner] version command: docker run --env LANG=C --rm --user 1001:123 --quiet --rm quay.io/quarkus/ubi-quarkus-mandrel-builder-image:jdk-17 --version
[INFO] [io.quarkus.deployment.pkg.steps.GraalVM] lines before filtering:
unknown flag: --quiet
See 'docker run --help'.

According to the documentation (docs.docker.com), the flag should exist. I'll remove the --quiet from run and pull again.

@turing85 turing85 force-pushed the feature/quiet-docker-command-for-determining-java-version branch from f996e16 to f1cffe1 Compare June 27, 2023 14:03
@turing85 turing85 changed the title Initialize build runner, make image pull- and GraalVM-version container commands quiet Initialize build runner in AwtProcessor before usage Jun 27, 2023
@turing85 turing85 force-pushed the feature/quiet-docker-command-for-determining-java-version branch from f1cffe1 to b4e7c2d Compare June 27, 2023 14:07
@turing85
Copy link
Contributor Author

@gsmet is this Pr still relevant, or can it be closed?

@gsmet gsmet force-pushed the feature/quiet-docker-command-for-determining-java-version branch from b4e7c2d to d7eda21 Compare September 22, 2023 14:22
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.

I think what we are doing in AwtProcessor is not really in line with what we should do. Or we should initialize things differently.

Let's merge your PR if CI passes but ideally I think we should rework this to avoid having all these quirks in the build steps.

@gsmet gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/backport-3.2 triage/backport-3.3? labels Sep 22, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Sep 22, 2023

✔️ 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.

@gsmet gsmet merged commit 68c2af1 into quarkusio:main Sep 22, 2023
@quarkus-bot quarkus-bot bot added this to the 3.5 - main milestone Sep 22, 2023
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label Sep 22, 2023
@gsmet gsmet modified the milestones: 3.5 - main, 3.4.2 Oct 3, 2023
@turing85 turing85 deleted the feature/quiet-docker-command-for-determining-java-version branch March 6, 2024 20:40
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.

3 participants