Bug 207012 – [build-webkit] Compare with cmakeargs and unhandled to detect configuration change

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Basuke Suzuki]

If --cmakeargs are passed, usually that means CMake Cache should be invalidated and do clean build.

[Basuke Suzuki]

Also command line options can change the configuration such as --no-ninja. Add them also.

[Basuke Suzuki]

Created attachment 389275 [details]
PATCH

[Stephan Szabo]

Comment on attachment 389275 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=389275&action=review

> Tools/Scripts/build-webkit:309
> +    my @buildArgs = (@featureArgs, @cmakeArgs, @ARGV);

This seems like it'd be prone to false positives for removal if I'm reading the code correctly.
It seems like the check is order dependent, so specifying the same cmake args in different orders seems like it would result in a positive to remove.
And something similar with feature args, where if a feature arg was -DFOO=ON, a run with no cmake args and one with a cmake arg setting -DFOO=ON (i.e. the value it already had) seems like it would result in a positive to remove.

[Stephan Szabo]

Comment on attachment 389275 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=389275&action=review

>> Tools/Scripts/build-webkit:309
>> +    my @buildArgs = (@featureArgs, @cmakeArgs, @ARGV);
> 
> This seems like it'd be prone to false positives for removal if I'm reading the code correctly.
> It seems like the check is order dependent, so specifying the same cmake args in different orders seems like it would result in a positive to remove.
> And something similar with feature args, where if a feature arg was -DFOO=ON, a run with no cmake args and one with a cmake arg setting -DFOO=ON (i.e. the value it already had) seems like it would result in a positive to remove.

I should add that sorting the cmake args (or feature + cmake args) would be insufficient since you could specify -DFOO=ON -DFOO=OFF which is different from "-DFOO=OFF -DFOO=ON" while "-DFOO=ON -DBAR=OFF" seems like it's the same as "-DBAR=OFF -DFOO=ON".
Also, thinking about it - mixing ARGV with the cmake/feature args seems like a problem if you can have an item in @ARGV that looked like -DFOO=ON, because you might get false negatives between build-webkit --cmakeargs="-DFOO=ON" and build-webkit -DFOO=ON.

[Basuke Suzuki]

(In reply to Stephan Szabo from comment #4)
> Comment on attachment 389275 [details]
> PATCH
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389275&action=review
> 
> >> Tools/Scripts/build-webkit:309
> >> +    my @buildArgs = (@featureArgs, @cmakeArgs, @ARGV);
> > 
> > This seems like it'd be prone to false positives for removal if I'm reading the code correctly.
> > It seems like the check is order dependent, so specifying the same cmake args in different orders seems like it would result in a positive to remove.
> > And something similar with feature args, where if a feature arg was -DFOO=ON, a run with no cmake args and one with a cmake arg setting -DFOO=ON (i.e. the value it already had) seems like it would result in a positive to remove.
> 
> I should add that sorting the cmake args (or feature + cmake args) would be insufficient since you could specify
> -DFOO=ON -DFOO=OFF which is different from "-DFOO=OFF -DFOO=ON" while "-DFOO=ON -DBAR=OFF" seems like it's the same
> as "-DBAR=OFF -DFOO=ON".
> Also, thinking about it - mixing ARGV with the cmake/feature args seems like a problem if you can have an item in
> @ARGV that looked like -DFOO=ON, because you might get false negatives between build-webkit --cmakeargs="-DFOO=ON"
> and build-webkit -DFOO=ON.

Yes, this patch is far from ideal, but rebuilding caused by false positive is much better than mis-configured build result, don't you think so?
But anyway sorting should be added.

[Carlos Alberto Lopez Perez]

Comment on attachment 389275 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=389275&action=review

>>>> Tools/Scripts/build-webkit:309
>>>> +    my @buildArgs = (@featureArgs, @cmakeArgs, @ARGV);
>>> 
>>> This seems like it'd be prone to false positives for removal if I'm reading the code correctly.
>>> It seems like the check is order dependent, so specifying the same cmake args in different orders seems like it would result in a positive to remove.
>>> And something similar with feature args, where if a feature arg was -DFOO=ON, a run with no cmake args and one with a cmake arg setting -DFOO=ON (i.e. the value it already had) seems like it would result in a positive to remove.
>> 
>> I should add that sorting the cmake args (or feature + cmake args) would be insufficient since you could specify -DFOO=ON -DFOO=OFF which is different from "-DFOO=OFF -DFOO=ON" while "-DFOO=ON -DBAR=OFF" seems like it's the same as "-DBAR=OFF -DFOO=ON".
>> Also, thinking about it - mixing ARGV with the cmake/feature args seems like a problem if you can have an item in @ARGV that looked like -DFOO=ON, because you might get false negatives between build-webkit --cmakeargs="-DFOO=ON" and build-webkit -DFOO=ON.
> 
> Yes, this patch is far from ideal, but rebuilding caused by false positive is much better than mis-configured build result, don't you think so?
> But anyway sorting should be added.

I like the idea of the patch, but I'm unsure about adding ARGV.

You mention that is for use-cases like "--no-ninja", but that option is removed from ARGV as soon as it checked on the canUseNinja() call above for the case of "if (isCMakeBuild() && !isAnyWindows()) {"

I suggest to move the code for cleaning the cmake cache above that check

[Basuke Suzuki]

Comment on attachment 389275 [details]
PATCH

View in context: https://bugs.webkit.org/attachment.cgi?id=389275&action=review

>>>>> Tools/Scripts/build-webkit:309
>>>>> +    my @buildArgs = (@featureArgs, @cmakeArgs, @ARGV);
>>>> 
>>>> This seems like it'd be prone to false positives for removal if I'm reading the code correctly.
>>>> It seems like the check is order dependent, so specifying the same cmake args in different orders seems like it would result in a positive to remove.
>>>> And something similar with feature args, where if a feature arg was -DFOO=ON, a run with no cmake args and one with a cmake arg setting -DFOO=ON (i.e. the value it already had) seems like it would result in a positive to remove.
>>> 
>>> I should add that sorting the cmake args (or feature + cmake args) would be insufficient since you could specify -DFOO=ON -DFOO=OFF which is different from "-DFOO=OFF -DFOO=ON" while "-DFOO=ON -DBAR=OFF" seems like it's the same as "-DBAR=OFF -DFOO=ON".
>>> Also, thinking about it - mixing ARGV with the cmake/feature args seems like a problem if you can have an item in @ARGV that looked like -DFOO=ON, because you might get false negatives between build-webkit --cmakeargs="-DFOO=ON" and build-webkit -DFOO=ON.
>> 
>> Yes, this patch is far from ideal, but rebuilding caused by false positive is much better than mis-configured build result, don't you think so?
>> But anyway sorting should be added.
> 
> I like the idea of the patch, but I'm unsure about adding ARGV.
> 
> You mention that is for use-cases like "--no-ninja", but that option is removed from ARGV as soon as it checked on the canUseNinja() call above for the case of "if (isCMakeBuild() && !isAnyWindows()) {"
> 
> I suggest to move the code for cleaning the cmake cache above that check

Thanks for the comment. I'll rework this with latest code this afternoon.

[Basuke Suzuki]

Created attachment 410273 [details]
PATCH

[Basuke Suzuki]

Created attachment 410277 [details]
PATCH

[Basuke Suzuki]

Created attachment 410278 [details]
PATCH

[Basuke Suzuki]

So to collect all possible configuration change, I chose to copy original argv and treat them as the change. `cmakeargs` is also one of them so that there's no need to treat it as special.

[Carlos Alberto Lopez Perez]

Comment on attachment 410278 [details]
PATCH

LGTM!

[EWS]

Committed r268134: <https://trac.webkit.org/changeset/268134>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 410278 [details].

[Radar WebKit Bug Importer]

<rdar://problem/70056353>

[Basuke Suzuki]

(In reply to Carlos Alberto Lopez Perez from comment #12)
> Comment on attachment 410278 [details]
> PATCH
> 
> LGTM!

Thanks!