207045 2020-01-31 07:44:15 -0800 build-webkit: SDKROOT and ARCHS should override command-line options 2020-01-31 15:56:27 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified NEW https://bugs.webkit.org/show_bug.cgi?id=206620 P2 Normal --- 1 jbedard jbedard aakash_jain ap dbates ews-watchlist wilander oldest_to_newest 1613357 0 jbedard 2020-01-31 07:44:15 -0800 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. 1613360 1 389349 jbedard 2020-01-31 07:48:48 -0800 Created attachment 389349 Patch 1613375 2 389349 ap 2020-01-31 08:34:39 -0800 Comment on attachment 389349 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. 1613377 3 ap 2020-01-31 08:38:14 -0800 > 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. 1613392 4 jbedard 2020-01-31 09:08:03 -0800 (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. 1613401 5 389358 jbedard 2020-01-31 09:20:36 -0800 Created attachment 389358 Patch 1613402 6 ap 2020-01-31 09:21:41 -0800 > 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. 1613433 7 jbedard 2020-01-31 09:50:15 -0800 (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 1613566 8 ap 2020-01-31 13:47:10 -0800 Pretty sure that you are talking about an empty set of people here. 1613581 9 jbedard 2020-01-31 14:13:05 -0800 (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. 1613636 10 ap 2020-01-31 15:56:27 -0800 This reason is that all build-webkit options are options. There is no need to start using arguments, environment variables, and such. 389349 2020-01-31 07:48:48 -0800 2020-01-31 09:20:34 -0800 Patch bug-207045-20200131074847.patch text/plain 2178 jbedard SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI1NTQ5NSkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDI2IEBACisyMDIwLTAxLTMxICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNv bT4KKworICAgICAgICBidWlsZC13ZWJraXQ6IFNES1JPT1QgYW5kIEFSQ0hTIHNob3VsZCBvdmVy cmlkZSBjb21tYW5kLWxpbmUgb3B0aW9ucworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA3MDQ1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdGRpcnMucG06CisgICAgICAgIChkZXRl cm1pbmVBcmNoaXRlY3R1cmUpOiBBbGxvdyBBUkNIUyBvcHRpb24gdG8gb3ZlcnJpZGUgZGVmYXVs dC4KKyAgICAgICAgKGRldGVybWluZVhjb2RlU0RLKTogQWxsb3cgU0RLUk9PVCBvcHRpb24gdG8g b3ZlcnJpZGUgZGVmYXVsdC4KKworMjAyMC0wMS0zMSAgSm9uYXRoYW4gQmVkYXJkICA8amJlZGFy ZEBhcHBsZS5jb20+CisKKyAgICAgICAgTWFrZWZpbGVzIHNob3VsZCBtYWtlIHdlYmtpdGRpcnMg YXdhcmUgb2YgdGhlIEFyY2hpdGVjdHVyZSwgU0RLCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJr aXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMDY2MjAKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JP RFkgKE9PUFMhKS4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0ZGlycy5wbToKKyAgICAgICAg KGRldGVybWluZUFyY2hpdGVjdHVyZSk6IENoZWNrIGFyZ3VtZW50cyBmb3IgdXNlciBzcGVjaWZp ZWQgYXJjaGl0ZWN0dXJlcy4KKyAgICAgICAgKGRldGVybWluZVhjb2RlU0RLKTogQ2hlY2sgZm9y IFNES1JPT1Q9KiBpbiBwYXNzZWQgYXJndW1lbnRzIHNvIHRoYXQgd2UgY2FuCisgICAgICAgIGFw cGx5IHRoZSBjb3JyZWN0IGFyZ3VtZW50cyBmb3IgYSBzcGVjaWZpYyBTREsuCisKIDIwMjAtMDEt MzAgIEppZXdlbiBUYW4gIDxqaWV3ZW5fdGFuQGFwcGxlLmNvbT4KIAogICAgICAgICBVbnJldmll d2VkLCBhbm90aGVyIHNwZWN1bGF0aXZlIHRlc3QgZml4IGFmdGVyIHIyNTUwNDEKSW5kZXg6IFRv b2xzL1NjcmlwdHMvd2Via2l0ZGlycy5wbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9TY3JpcHRzL3dl YmtpdGRpcnMucG0JKHJldmlzaW9uIDI1NTQ5MykKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0ZGly cy5wbQkod29ya2luZyBjb3B5KQpAQCAtMzY4LDcgKzM2OCw3IEBAIHN1YiBkZXRlcm1pbmVBcmNo aXRlY3R1cmUKICAgICAgICAgfQogICAgICAgICBpZiAoJGFyY2hpdGVjdHVyZSkgewogICAgICAg ICAgICAgY2hvbXAgJGFyY2hpdGVjdHVyZTsKLSAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgfSBl bHNpZiAoIWNoZWNrRm9yQXJndW1lbnRBbmRSZW1vdmVGcm9tQVJHVkdldHRpbmdWYWx1ZSgiQVJD SFMiLCBcJGFyY2hpdGVjdHVyZSkpIHsKICAgICAgICAgICAgIGlmIChub3QgZGVmaW5lZCAkeGNv ZGVTREsgb3IgJHhjb2RlU0RLID1+IC9eKFwvJHxtYWNvc3gpLykgewogICAgICAgICAgICAgICAg IG15ICRzdXBwb3J0czY0Qml0ID0gYHN5c2N0bCAtbiBody5vcHRpb25hbC54ODZfNjRgOwogICAg ICAgICAgICAgICAgIGNob21wICRzdXBwb3J0czY0Qml0OwpAQCAtNTg3LDYgKzU4NywxMSBAQCBz dWIgZGV0ZXJtaW5lWGNvZGVTREsKICAgICAgICAgJHhjb2RlU0RLID0gJHNkazsKICAgICAgICAg cmV0dXJuOwogICAgIH0KKyAgICBpZiAoY2hlY2tGb3JBcmd1bWVudEFuZFJlbW92ZUZyb21BUkdW R2V0dGluZ1ZhbHVlKCJTREtST09UIiwgXCRzZGspKSB7CisgICAgICAgICR4Y29kZVNESyA9ICRz ZGs7CisgICAgICAgIHJldHVybjsKKyAgICB9CisKICAgICBpZiAoY2hlY2tGb3JBcmd1bWVudEFu ZFJlbW92ZUZyb21BUkdWKCItLWRldmljZSIpIHx8IGNoZWNrRm9yQXJndW1lbnRBbmRSZW1vdmVG cm9tQVJHVigiLS1pb3MtZGV2aWNlIikpIHsKICAgICAgICAgJHhjb2RlU0RLIHx8PSAiaXBob25l b3MiOwogICAgIH0K 389358 2020-01-31 09:20:36 -0800 2020-01-31 13:47:17 -0800 Patch bug-207045-20200131092035.patch text/plain 2061 jbedard SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDI1NTUwMCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE5IEBACisyMDIwLTAxLTMxICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNv bT4KKworICAgICAgICBidWlsZC13ZWJraXQ6IFNES1JPT1QgYW5kIEFSQ0hTIHNob3VsZCBvdmVy cmlkZSBjb21tYW5kLWxpbmUgb3B0aW9ucworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9MjA3MDQ1CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChP T1BTISkuCisKKyAgICAgICAgVGhpcyBjaGFuZ2UgZW5zdXJlIHRoYXQgaWYgYSB1c2VyIHBhc3Nl cyBBUkNIUyB0byBidWlsZC13ZWJraXQsIHRoZSB1c2VyJ3Mgc3BlY2lmaWNhdGlvbiB3aWxsCisg ICAgICAgIG92ZXJyaWRlIHRoZSBwbGF0Zm9ybSdzIGRlZmF1bHQgYXJjaGl0ZWN0dXJlLiBGb3Ig ZXhhbXBsZToKKyAgICAgICAgICAgIEJ1aWxkLXdlYmtpdCAtLWlvcy1kZXZpY2UgQVJDSFM9YXJt NjRlCisgICAgICAgIHNob3VsZCBwYXNzIEFSQ0hTPWFybTY0ZSB0byBYY29kZSwgaW5zdGVhZCBv ZiBhcm02NCAodGhlIGRlZmF1bHQgZm9yIFNES1JPT1Q9aXBob25lb3MpCisKKyAgICAgICAgKiBT Y3JpcHRzL3dlYmtpdGRpcnMucG06CisgICAgICAgIChkZXRlcm1pbmVBcmNoaXRlY3R1cmUpOiBB bGxvdyBBUkNIUyBvcHRpb24gdG8gb3ZlcnJpZGUgZGVmYXVsdCBmcm9tIHNwZWNpZmllZCBwbGF0 Zm9ybS4KKyAgICAgICAgKGRldGVybWluZVhjb2RlU0RLKTogQWxsb3cgU0RLUk9PVCBvcHRpb24g dG8gb3ZlcnJpZGUgZGVmYXVsdCBmcm9tIHNwZWNpZmllZCBwbGF0Zm9ybS4KKwogMjAyMC0wMS0z MSAgSm9uYXRoYW4gQmVkYXJkICA8amJlZGFyZEBhcHBsZS5jb20+CiAKICAgICAgICAgcmVzdWx0 cy53ZWJraXQub3JnOiBIYW5kbGUgbW9kaWZpZWQgZmlyZXdhbGwgcnVsZXMKSW5kZXg6IFRvb2xz L1NjcmlwdHMvd2Via2l0ZGlycy5wbQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9TY3JpcHRzL3dlYmtp dGRpcnMucG0JKHJldmlzaW9uIDI1NTUwMCkKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0ZGlycy5w bQkod29ya2luZyBjb3B5KQpAQCAtMzY4LDcgKzM2OCw3IEBAIHN1YiBkZXRlcm1pbmVBcmNoaXRl Y3R1cmUKICAgICAgICAgfQogICAgICAgICBpZiAoJGFyY2hpdGVjdHVyZSkgewogICAgICAgICAg ICAgY2hvbXAgJGFyY2hpdGVjdHVyZTsKLSAgICAgICAgfSBlbHNlIHsKKyAgICAgICAgfSBlbHNp ZiAoIWNoZWNrRm9yQXJndW1lbnRBbmRSZW1vdmVGcm9tQVJHVkdldHRpbmdWYWx1ZSgiQVJDSFMi LCBcJGFyY2hpdGVjdHVyZSkpIHsKICAgICAgICAgICAgIGlmIChub3QgZGVmaW5lZCAkeGNvZGVT REsgb3IgJHhjb2RlU0RLID1+IC9eKFwvJHxtYWNvc3gpLykgewogICAgICAgICAgICAgICAgIG15 ICRzdXBwb3J0czY0Qml0ID0gYHN5c2N0bCAtbiBody5vcHRpb25hbC54ODZfNjRgOwogICAgICAg ICAgICAgICAgIGNob21wICRzdXBwb3J0czY0Qml0OwpAQCAtNTg3LDYgKzU4NywxMSBAQCBzdWIg ZGV0ZXJtaW5lWGNvZGVTREsKICAgICAgICAgJHhjb2RlU0RLID0gJHNkazsKICAgICAgICAgcmV0 dXJuOwogICAgIH0KKyAgICBpZiAoY2hlY2tGb3JBcmd1bWVudEFuZFJlbW92ZUZyb21BUkdWR2V0 dGluZ1ZhbHVlKCJTREtST09UIiwgXCRzZGspKSB7CisgICAgICAgICR4Y29kZVNESyA9ICRzZGs7 CisgICAgICAgIHJldHVybjsKKyAgICB9CisKICAgICBpZiAoY2hlY2tGb3JBcmd1bWVudEFuZFJl bW92ZUZyb21BUkdWKCItLWRldmljZSIpIHx8IGNoZWNrRm9yQXJndW1lbnRBbmRSZW1vdmVGcm9t QVJHVigiLS1pb3MtZGV2aWNlIikpIHsKICAgICAgICAgJHhjb2RlU0RLIHx8PSAiaXBob25lb3Mi OwogICAgIH0K