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
Allows Linux-only extensions on Windows with Linux builder image #28412
Conversation
This comment has been minimized.
This comment has been minimized.
Hmmm, I wonder if I broke that on Mac?
Perhaps it should be explicitly matched to Win? |
@@ -875,7 +875,8 @@ public NativeImageInvokerInfo build() { | |||
} | |||
|
|||
if (unsupportedOSes != null && !unsupportedOSes.isEmpty()) { | |||
final String errs = unsupportedOSes.stream().filter(o -> o.os.active).map(o -> o.error) | |||
final String errs = unsupportedOSes.stream().filter(o -> o.os.active && !nativeConfig.isContainerBuild()) |
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 if the unsupported OS is Linux?
IMHO you're making a general infrastructure extremely specific with this change.
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.
If the extension is Windows-specific and the unsupported OS is Linux, then my patch doesn't work when executed in a Linux container on a Windows host.
I am making a baold assumption that we have only Linux builder images (e.g. no Windows Server Core based ones) and that all extensions work on Linux as a baseline.
Coming from that, if you run on Mac or Windows, you are always going to be using a Linux builder image...
I can add a logic to the Native Image Config that would report the Builder Image's OS and then let the UnsupportedOS compare to that.
I think it is an overkill right now when it comes to OS. On the other hand, it might be useful as a check for the architecture. Supporting/Not supporting amd64 vs. aarch64 builder image on Mac comes to mind.
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.
So yes, the assumption of Linux container can be made. We make it in other places.
My problem with your patch:
- let's assume a feature is unsupported on Linux
- let's assume we are on Linux in a container build
o.os.active
is true!nativeConfig.isContainerBuild()
is false- this feature doesn't work as expected
I think you will have to special case Linux somehow for this feature to keep being working in all cases.
The condition should be something like: (nativeConfig.isContainerBuild() && o.os == linux) || o.os.active
(in pseudo code).
Does it make sense to you?
BTW, I approved the PR by mistake, it was a request for changes.
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.
Ack. I got the point. Lemme take a second look....and I guess also to add a test so as the context of this change is not lost like tears in rain.
BTW: It seems that the time it takes to build Quarkus on I/O bound Windows VMs is getting slightly out of hand :-( Not sure what to do about it.
@gsmet
I also attempted to comment the "logic" so as we can remember this tomorrow.... |
...and it behaves on Linux as expected too. |
fixes #28335
Running on Windows host
There is a correct exception explaining that this is not going to work:
Running on Windows with podman
This is allowed now because the build happens in Linux userland: