-
-
Notifications
You must be signed in to change notification settings - Fork 255
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
making build -D working with podman #3796
Conversation
Currenlty the build fails in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial thoughts - I haven't gone through all of the functionality yet but I'll do that as a second pass :-)
@@ -45,7 +45,7 @@ as we can generate valid dockerfile for it): | |||
|
|||
```bash | |||
./makejdk-any-platform.sh --docker --clean-docker-build jdk8u | |||
./makejdk-any-platform.sh --docker --clean-docker-build --build-variant openj9 jdk11u | |||
./makejdk-any-platform.sh --podman --clean-docker-build --build-variant openj9 jdk11u |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to make this -D
now?
./makejdk-any-platform.sh --podman --clean-docker-build --build-variant openj9 jdk11u | |
./makejdk-any-platform.sh -D --clean-docker-build --build-variant openj9 jdk11u |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had left -D
to autodetect. Id there is podman, it will be used. If not, docekr will be used. Reason for this fallback is that if you have podman, you have also docker aliases. But if you have docker, you do no thave podman aliases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep understood - I just wasn't sure why this example was explicitly switched to --podman
instead of showing the "autodetect" version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I jsut wanted to keep all three there. If they should be reuced, jsut say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a slight preference fur using the generic -D
, although I quite like the multiple examples too, so no real preference with your explaination :-)
@@ -270,8 +270,14 @@ function parseConfigurationArguments() { | |||
"--destination" | "-d" ) | |||
BUILD_CONFIG[TARGET_DIR]="$1"; shift;; | |||
|
|||
"--docker" | "-D" ) | |||
BUILD_CONFIG[USE_DOCKER]="true";; | |||
"-D" ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the reason for using this USE_DOCKER
variable instead of the exsting DOCKER
one. It means we now have lines elsewhere like:
${BUILD_CONFIG[DOCKER]} ${BUILD_CONFIG[USE_DOCKER]}
which aren't as clear as they could be when you look at those lines in isolation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are absolutely misleading, but they were already before. Before this goes in, I would like to rename ethem both, so they fit at least a bit to what they are doing. I have left them in for readability (although after all, it do not help.)
Originally the BUILD_CONFIG[USE_DOCKER] was true/false
and ${BUILD_CONFIG[DOCKER]} was "docker" or "sudo docker"
. Now the BUILD_CONFIG[USE_DOCKER] is false/docker/podman
and ${BUILD_CONFIG[DOCKER]} is sudo or nothing
The rname should be BUILD_CONFIG[CONTAINER_COMMAND]
and ${BUILD_CONFIG[SUDO_CONTAINER]}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was long hesitating wheter to keep using USE_DOCKER
true/false flag, and keep command in DOCKER
with docker/podman/sudo docker/sudo podman
and decided with slight 55:45 overwhelm to use USE_DOCKER as command and DOCKER as sudo preffix, with intention to rename at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-m-leonard Am I correct in saying that thse parameters are not defined at any point in the jenkins jobs so changing them here should be "safe"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually - main reason to swithc, was to add more flexibility for possible "sudo" fixes. Eg to allow choice sudo/run0/doas ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrew-m-leonard Am I correct in saying that thse parameters are not defined at any point in the jenkins jobs so changing them here should be "safe"?
Great remark. No idea. Then the rename will go as spearte PR onc.. if... those changese ever goes in. TYVM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ping @andrew-m-leonard ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I was long hesitating wheter to keep using USE_DOCKER true/false flag, and keep command in DOCKER with docker/podman/sudo docker/sudo podman and decided with slight 55:45 overwhelm to use USE_DOCKER as command and DOCKER as sudo preffix, with intention to rename at the end.
Yeah makes sense - I think if we do the renames so that it's a little more comprehensible to new people looking at the scripts.
${BUILD_CONFIG[DOCKER]} "${BUILD_CONFIG[USE_DOCKER]}"
Looks a bit odd in isolation so something like
${BUILD_CONFIG[SUDO_CONTAINER]} BUILD_CONFIG[CONTAINER_COMMAND]
would be a lot clearer, so if we can get some of these renames in (and hope this PR rebases on master nicely!) then I'm personally good with putting this in if Andrew agrees.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. The renaming to somethnig like that will be necessary. Currently it is terribly unreadable.
yes, the renaming is missing due to rebases :) .. no it do not rebase smoothly, but I will elaborate !
CONTAINER_COMMAND
sounds like clear choice. CONTAINER_AS_ROOT
may be better if we want to prevent another renaming in future (and PRIVLIDGED_CONTAINER
is jsut wrong). But currently it is indeed jsut sudo. So SUDO_CONTAINER
would do.
This comment was marked as off-topic.
This comment was marked as off-topic.
It seems I'm on wrong track/ |
@sxa hi! Sorry for dummy question, but I have to be missing something. The docket build scripts, as I read it, nowhere clone/copy/mount the jdk sources. Do you recall how it was designed to obtain them? (the |
I added few more dirs.. but that is getting back to my weeird question;
That it do not obtains sources... |
Just a nit, exceptnot working, this PR is doing its job. Now both podman and docker are working/failing the same. Only the wrapper do not work as expected. Maybe for pretty long time. I may try to contineu, but I need to knwo if there is any interest in that. |
7a33d32
to
d8a44d6
Compare
@sxa @andrew-m-leonard @karianna
Any idea? |
I think it's useful to have that operational (especially since we mention this in the top level README.md in this repository), although perhaps we should just direct people to our docker images, such as |
Pls, note #3855 for my continuous thoughts flow. If there is any better place where to put podman and fedora/rhel support, let me know. |
NO better suggestions - that issue LGTM. |
Note for myself for tomorrow:
Is missing ot make docker "pass" to die on
Which was also resolve din this PR. I keep forgetting to add that condition. (as it changed in this pr) |
both
are now passing on my docker (first) and podman (second) fedora40 vm \o/ Ok to rename and merge?-) |
configureBuild.sh
Outdated
@@ -385,5 +385,7 @@ configure_build() { | |||
setWorkingDirectory | |||
configureMacFreeFont | |||
setMakeArgs | |||
setBootJdk | |||
if [ "${BUILD_CONFIG[USE_DOCKER]}" == false ] ; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think from looking at the other changes, USE_DOCKER is either "docker" or "podman", and not "false"?
Which leads to the point that the variable name is confusing...?
Maybe we should add a new variable, say "CONTAINER_CMD" (docker|podman), and if we need a boolean, call that "USE_CONTAINER" (true|false) ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanx!
both variables must be renamed as last stage of this PR (I had nto yet done it to avoid broken rebases).
It is not visdibel in changeset, but defautl value of BUILD_CONFIG[USE_DOCKER] is really false.:
temurin-build/sbin/common/config_init.sh
Line 579 in 27b8c5c
BUILD_CONFIG[USE_DOCKER]=${BUILD_CONFIG[USE_DOCKER]:-false} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See also #3796 (comment) and #3796 (comment)
…${BUILD_CONFIG[USE_DOCKER]}" Originally, this patch started to fix properly quote for safety (thanx linter), I foudn that on sme pleaces, original ${BUILD_CONFIG[DOCKER]} was not repalced by new tandem. ${BUILD_CONFIG[DOCKER]} was 'docker' or 'sudo docker'. I had split it, so ${BUILD_CONFIG[DOCKER]} is sudo or nothing and ${BUILD_CONFIG[USE_DOCKER]}" is docker or podman. The variables have to be renamed at the end to adhere more to theirs purposes.
all sub dirs should be then created by follwoing prepare-workspace
Ok, renaming vars! |
BUILD_CONFIG[USE_DOCKER]-> BUILD_CONFIG[CONTAINER_COMMAND] BUILD_CONFIG[DOCKER] -> BUILD_CONFIG[CONTAINER_AS_ROOT] BUILD_CONFIG[USE_DOCKER] values: false, podman, docker BUILD_CONFIG[DOCKER] values: sudo,empty string Other docker based variables which are globally container bound remained intact (CLEAN_DOCKER_BUILD, DEBUG_DOCKER, DOCKER_FILE_PATH...)
renamed, local docker and podman builds running |
Podman build ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks good now
tyvm! |
Also docker buld passed. Now trtying 8u too |
hm. Both jdk8u builds failed in free type. Podman, free type build passed, jdk configure:
docker, cionfigure of freetype fails:
However it seems, this happens also on master (verified). So the changeset seesm to not be guilty. The only "what?" is different failure in docker/podman. |
Hmmm was that using the same OS image between podman and docker? That error looks to me like it was run with a bourne shell instead of base (for example Ubuntu's default |
Indeed. I had not lookled into it more, as it fails both before and aftr this PR the same. But It remains on my todo: #3863 |
Gosh. While trying all othjer jdks (11-22) where they all passed (both with cloning and with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now - thanks for this :-)
This is resolving the small differences between podman and docker. In systems with both, it allows to select the concrete one.