If the user passes ARCHS or SDKROOT to build-webkit, it should override the defaults set by the command line. This is especially important for ARCHS since there are often multiple valid architectures for a given platform.
Created attachment 389349 [details] Patch
Comment on attachment 389349 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=389349&action=review > Tools/ChangeLog:3 > + build-webkit: SDKROOT and ARCHS should override command-line options What combination of options is this fixing? That's worth explaining in ChangeLog. > Tools/ChangeLog:14 > + Makefiles should make webkitdirs aware of the Architecture, SDK Double ChangeLog.
> If the user passes ARCHS or SDKROOT to build-webkit, it should override the > defaults set by the command line. Seeing the explanation here, I'm not sure if I agree. I think that build-root shouldn't support these. Make options should be passed via --makeargs, and build-webkit options should all be proper command line options with "--". Doing otherwise would be mysterious and inconsistent.
(In reply to Alexey Proskuryakov from comment #3) > > If the user passes ARCHS or SDKROOT to build-webkit, it should override the > > defaults set by the command line. > > Seeing the explanation here, I'm not sure if I agree. I think that > build-root shouldn't support these. Make options should be passed via > --makeargs, and build-webkit options should all be proper command line > options with "--". Doing otherwise would be mysterious and inconsistent. There is a good case for not doing SDKROOT (since --ios-simulator and --ios-device set it). But failing to look for ARCHS is actually a pretty big correctness problem. This means that someone ran 'build-webkit --ios-device ARCHS=arm64e', we would pass this: ARCHS=arm64 ARCHS=arm64e to Xcode. Which is wrong. We could add a --architecture option, but it feels better to just make this work with ARCHS. This is really important for watchOS, where there are 3 valid architectures, and for iOS if we want the default build to include both arm64 and arm64e.
Created attachment 389358 [details] Patch
> We could add a --architecture option, but it feels better to just make this work with ARCHS. Can you elaborate on why this feels better to you? That's exactly what I don't like due to inconsistency with other options.
(In reply to Alexey Proskuryakov from comment #6) > > We could add a --architecture option, but it feels better to just make this work with ARCHS. > > Can you elaborate on why this feels better to you? That's exactly what I > don't like due to inconsistency with other options. We would be asking users to learn a new option which didn't exist before, and it's likely that those interested in setting architecture are already passing ARCHS to build-webkit. Use --<platform> options in build-webkit makes some sense because run-webkit-tests uses the same style. The only established style we have for architecture that I've encountered before is ARCHS. Even our builders use it: https://build.webkit.org/builders/Apple%20iOS%2013%20Release%20%28Build%29/builds/3826/steps/compile-webkit/logs/stdio
Pretty sure that you are talking about an empty set of people here.
(In reply to Alexey Proskuryakov from comment #8) > Pretty sure that you are talking about an empty set of people here. Anyone who has been copying build commands from bots would be building with ARCHS. I think we need a compelling reason to use --architecture over ARCHS=, since we don't have any scripts using --architecture at the moment.
This reason is that all build-webkit options are options. There is no need to start using arguments, environment variables, and such.