Bug 207045

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.