Adds -H:+GenerateBuildArtifactsFile, copies .so from remote container #41201
Conversation
|
Open for discussion, I think I could add a test, something like to test that this PR keeps working and that this #35718 doesn't break. It would create two additional builds, taking more time though. Perhaps a thing suited for quickstarts better. Not sure. |
|
Do we need both those new integration tests? Can't we just have one? |
...ent/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildRemoteContainerRunner.java
Outdated
Show resolved
Hide resolved
| } | ||
| try (BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()))) { | ||
| return reader.readLine(); | ||
| return reader.lines().toList(); |
There was a problem hiding this comment.
What's the motivation behind this change? I understand that it's more generic this way but since we only use the method in a single place where this is not needed it seems to unnecessarily (for the time being) complicate things.
There was a problem hiding this comment.
@zakkak I used that to read the output of exec for find to list the .so files in the started container. Then I abandoned that approach in favor of using the -H:+GenerateBuildArtifactsFile. So strictly speaking, we can leave in the thing that returns only the first line of output, however confusing that might be. Would rename the method though then, to ...readOneLineOutput...
| copyFromContainerVolume(outputDir, "sources", "Failed to copy sources from container volume back to the host."); | ||
| String symbols = String.format("%s.debug", nativeImageName); | ||
| copyFromContainerVolume(outputDir, symbols, "Failed to copy debug symbols from container volume back to the host."); | ||
| copyFromContainerVolume(outputDir, "sources", | ||
| "Failed to copy sources from container volume back to the host."); | ||
| final String symbols = String.format("%s.debug", nativeImageName); | ||
| copyFromContainerVolume(outputDir, symbols, | ||
| "Failed to copy debug symbols from container volume back to the host."); |
There was a problem hiding this comment.
Are these changes necessary?
There was a problem hiding this comment.
These changes are not necessary to fix the issue.
I wonder how to jam both into a single run. Perhaps we can. Like a project that has Could be one 👍 |
This comment has been minimized.
This comment has been minimized.
Test Lambda and remote container packaging
|
Commit 59b63bb adds a single new integration test that has in its application.properties: It tests that both #41020 is fixed and that #35718 has no regression. Tested as It passes, although it produces these Lambda server related warnings, I guess it could have something to do with the fact that the LambdaClient is deprecated...? I tried to ask about it on Zulip: https://quarkusio.zulipchat.com/#narrow/stream/187038-dev/topic/Testing.20AWS.20Lambda.20locally/near/444730456 |
|
@zakkak
I left the changes regarding reading output there, both the error/debug ones and the runCommandAndReadOutput. |
Thx. I originally thought this would be 1 line of :-D but then Source: https://docs.podman.io/en/latest/markdown/podman-cp.1.html#description
This PR deals the scenario when you run your build "remotely" in a container, rather than just calling native-image from a container. The OP had a problem with Gradle, but it's not Gradle specific. If you are building your runtime image using a Dockerfile, you still need to have a line in the Dockerfile that copies those files, e.g. https://github.com/quarkusio/quarkus/blob/main/integration-tests/awt/src/main/docker/Dockerfile.native#L10 Is there some kind of "Default" Dockerfile I can edit? It used to be a problem because not everybody was using GraalVM/Mandrel 23.0+ and older versions did not produce additional .so files. Docker's COPY of non-existing files then triggered errors. That required "hacks" like copying an unnecessary, but always present file, such as some .properties file, so as the COPY always copies something: WDYT? |
quarkus-bot
bot
added
the
area/infra-automation
Yeah I think we should go with it if we are sure we always have a least one properties file. Be aware that we might not have an The Dockerfiles template for newly generated projects are in |
It must have been one of the artifacts in target/, so I think
Gonna take a look. |
|
Ad "getting verticle", a message per my workstation core, why is it printing there? It's gonna be 256 messages on dualsocket Altra Max :) ...and indeed, I've juts tried on an 80 core machine and there is 80 |
Status for workflow
|
|
@Karm I guess you overcame the native image test you were having :) |
Well, I wish I had. I used the deprecated API LambdaClient, which feels suboptimal for a new test... |
|
Sure yeah, but we can figure it out later |
|
Hello, are there any action items for me? |
fixes #41020
This PR adds to all native builds the option
-H:+GenerateBuildArtifactsFilethat produces a small JSON file during the native-image build process. The JSON file contains a list of built artifacts.We use that file as a manifest of what was built and what we need to copy from the remote container where the remote build took place. If no such file exists or if there are no extra artifacts to copy, we silently do nothing.
What the new file looks like
Why don't you podman cp?
Podman cp does not support globs, i.e.
cp /project/*.sowon't work, which I found surprising so I duly noted that in the comments in the code so as to save time for others who might be tempted to refactor it later.