Add all flags to scanner discovery #158

jonahgraham · 2022-11-11T03:04:21Z

Fixes Bug 571722.

This change adds the ALL_FLAGS that does not limit tool options to
those declared as IOption::isForScannerDiscovery when launching the
compiler to discover compiler built-ins.

This is needed as many other flags, either entered manually in "Other
flags" or some of the existing flags with checkboxes such as "-ansi",
"-fPIC", and "-fstack-protector-all" which all affect scanner discovery
as they can all change what macros are built-in to the compiler.

The current solution has as a drawback that some settings, like -I and -D
then appear twice. For example in the "Includes" node in the "Project
Explorer"

My only reservation about this change is if there is an option
that can be specified successfully at build time, but when used
at scanner discovery time causes the compiler to fail, or return
incorrect results. Therefore I have added a new field,
excludeFromScannerDiscovery to tool options (buildDefinitions
extension point) that allows tool integrators to always exclude
a command line option from ALL_FLAGS. I have also added
a new "Other flags (excluded from discovery)" to the
"Miscellaneous" tab to allow compiler options to be entered
by the user.

TODO:

N&N entry
Provide PR to embed-cdt: Use new ALL_FLAGS feature of CDT for discovery eclipse-embed-cdt/eclipse-plugins#547
This PR includes some code cleanups in the parent commits, submit them separately Various code cleanups #160

github-actions · 2022-11-11T04:55:55Z

Test Results

    626 files     626 suites 47m 23s ⏱️
11 328 tests 11 194 ✔️ 134 💤 0 ❌
11 344 runs 11 210 ✔️ 134 💤 0 ❌

Results for commit 5b08c5d.

♻️ This comment has been updated with latest results.

jonahgraham · 2022-11-11T14:43:47Z

@jld01 @T-Svensson I would appreciate your thoughts on this while I work to make sure it doesn't regress anything - I wrote the N&N entries too:

Torbjorn-Svensson · 2022-11-11T18:18:42Z

NewAndNoteworthy/CDT-11.0.md

+## Scanner Discovery considering all flags
+
+The default for the scanner has changed to include _all_ command line options when querying the compiler for built-in includes and defines.
+This is because many more compiler options affect the includes and defines than those that were traditionally included when running the scanner, such as `-ansi`, `-fPIC`, `-fstack-protector-all`.


This is because a lot more..

Torbjorn-Svensson · 2022-11-11T18:19:59Z

NewAndNoteworthy/CDT-11.0.md

+The default for the scanner has changed to include _all_ command line options when querying the compiler for built-in includes and defines.
+This is because many more compiler options affect the includes and defines than those that were traditionally included when running the scanner, such as `-ansi`, `-fPIC`, `-fstack-protector-all`.
+
+To address this a new variable called `${ALL_FLAGS}` is included by default when performing scanner discovery.


To address this, a new variable called ${ALL_FLAGS} is included by default when performing scanner discovery.

Torbjorn-Svensson · 2022-11-11T18:27:10Z

...uilder.core.tests/tests/org/eclipse/cdt/managedbuilder/core/tests/ManagedBuildCoreTests.java

@@ -46,14 +43,6 @@ public ManagedBuildCoreTests(String name) {
 		super(name);
 	}

-	public static Test suite() {


Maybe this block should be removed in it's own commit?

Torbjorn-Svensson · 2022-11-11T18:31:16Z

build/org.eclipse.cdt.managedbuilder.core/schema/buildDefinitions.exsd

+         <attribute name="excludeFromScannerDiscovery" type="boolean">
+            <annotation>
+               <documentation>
+                  An optional field to allow the option to be excluded from scanner discovery's ${ALL_FLAGS} variable. The default is false.


Maybe it would be better to change this from "The default is false." to "The default is to include option in scanner discovery." or something like that?
False can be interpreted as either that it's excluded and included...

Torbjorn-Svensson · 2022-11-11T18:34:52Z

...eclipse.cdt.managedbuilder.core/src/org/eclipse/cdt/managedbuilder/internal/core/Option.java

@@ -578,6 +588,14 @@ protected void loadFromProject(ICStorageElement element) {
 			}
 		}

+		// isNotForScannerDiscovery
+		if (element.getAttribute(EXCLUDE_FROM_SCANNER_DISCOVERY) != null) {


Why are you fetching the same attribute 2 times and checking for null 2 times?

Torbjorn-Svensson · 2022-11-11T18:40:02Z

...org/eclipse/cdt/managedbuilder/language/settings/providers/AbstractBuiltinSpecsDetector.java

+	 * except for those specifically excluded from scanner discovery
+	 * with {@link IOption#isExcludedFromScannerDiscovery()}
+	 *
+	 * @param languageId - language ID.


I think you should drop the "-" here.

Torbjorn-Svensson · 2022-11-11T19:00:41Z

core/org.eclipse.cdt.ui/src/org/eclipse/cdt/internal/ui/newui/Messages.properties

@@ -101,7 +101,8 @@ AbstractPage_6=Configuration:\u0020
 AbstractPage_7=Exclude resource from build
 AbstractPage_8=Cannot apply
 AbstractPage_9=Internal error
-AbstractPage_rebuildIndex_question=Changes made will not be reflected in the index until it is rebuilt. Do you wish to rebuild it now?
+AbstractPage_Rebuild_Index=Rebuild Index
+AbstractPage_rebuildIndex_question=Some build settings changes may affect the index. These changes won't take effect until the index until it is rebuilt. Do you wish to rebuild it now?


Some changes to build settings may affect the index. These changes won't take effect until the index is rebuilt. Do you wish to rebuild it now?

jonahgraham · 2022-11-12T01:29:34Z

Thanks @Torbjorn-Svensson for the review. I agree with all your changes.

At the high level, is this change a good idea, or cause you any concern?

I am pretty confident about most of it, the only thing that concerns me is if I should change the default for new projects to include ${ALL_FLAGS} or leave it for affected users to change it.

Torbjorn-Svensson · 2022-11-12T12:00:18Z

At the high level, is this change a good idea, or cause you any concern?

I am pretty confident about most of it, the only thing that concerns me is if I should change the default for new projects to include ${ALL_FLAGS} or leave it for affected users to change it.

My first reaction was "why is this needed...", but after reading the entire PR and the changes in it, I suppose it makes senses.
If we are going to do the change, it should be done now as CDT11 contains breaking changes in other areas.
Maybe we should leave the default pattern to be the old one, but then I suppose we will never be able to offer the new pattern as the default value and we would sort of stay with the current limitations (the ones that this PR is suppose to resolve).
Do you think there is a good way to select the pattern on a high level without having to write it manually in the plugin.xml? Some property that will dynamically select the the new pattern if the toolchain description states that it's compatible but without knowing the actual pattern, or something like that? Or we just let our users do this one time switch when they upgrade to CDT11? I don't see this as a major issue anyway.

This change adds the ALL_FLAGS that does not limit tool options to those declared as IOption::isForScannerDiscovery when launching the compiler to discover compiler built-ins. This is needed as many other flags, either entered manually in "Other flags" or some of the existing flags with checkboxes such as "-ansi", "-fPIC", and "-fstack-protector-all" which all affect scanner discovery as they can all change what macros are built-in to the compiler. The current solution has as a drawback that some settings, like -I and -D then appear twice. For example in the "Includes" node in the "Project Explorer" My only reservation about this change is if there is an option that can be specified successfully at build time, but when used at scanner discovery time causes the compiler to fail, or return incorrect results. Therefore I have added a new field, excludeFromScannerDiscovery to tool options (buildDefinitions extension point) that allows tool integrators to always exclude a command line option from ALL_FLAGS. I have also added a new "Other flags (excluded from discovery)" to the "Miscellaneous" tab to allow compiler options to be entered by the user.

jonahgraham · 2022-11-21T00:49:46Z

I don't have confidence to change the default, so this latest version adds the new ${ALL_FLAGS} but make it not the default.

Do you think there is a good way to select the pattern on a high level without having to write it manually in the plugin.xml?

Short answer is that I didn't come up with a good way. So this feature is there for those that know about it (who read the N&N/docs).

Separately, it may be worth going to set useByScannerDiscovery to true for many more options so that people are less likely to fall for the problem in the first place. I am going to do that for embed-cdt instead of the previous PR (eclipse-embed-cdt/eclipse-plugins#547) which also has the advantage of working with older CDTs as embed-cdt has CDT 10 as minimum version.

Torbjorn-Svensson · 2022-11-21T17:55:07Z

I just had another thought about this.
Can't we simply flip the default value for useByScannerDiscovery from false to true.
Sure, there are plugins out there that want to be compatible with both CDT10 and CDT11 and this way, they just have to declare what settings that should not be part of the scanner logic. I mean, is there really a need for a new pattern?

jonahgraham · 2022-11-21T18:08:03Z

I think that has the same risks of breaking existing projects as making ALL_FLAGS the default. In practice it will mean that all existing options will be effectively useByScannerDiscovery == true. But unlike the ALL_FLAGS being default, there will be no way for users to get it to work if there is a conflict between the option and scanner discovery (except to move those options into the new option field I added, which isn't even possible for some enum type options)

Your proposal should have been the original design!

Torbjorn-Svensson · 2022-11-21T18:14:44Z

I guess you are right, but the ALL_FLAGS workaround means that after you do that, it's strictly CDT11 as the symbol is unavailable in CDT10. Isn't that worse of a break?

I'm not saying that we should do A or B, I'm just venting our options.

jonahgraham · 2022-11-21T18:39:56Z

Isn't that worse of a break?

I don't think that is a break at all. The change is backwards compatible, but not forwards compatible.

Torbjorn-Svensson · 2022-11-21T18:54:21Z

Isn't that worse of a break?

I don't think that is a break at all. The change is backwards compatible, but not forwards compatible.

I was thinking more about if someone adopts the ALL_FLAGS instead of FLAGS, then that project will not work with CDT10, while the solution to invert the default value for useByScannerDiscovery would work for both, but "forces" the toolchain integration provider to have the correct value on the options.

jonahgraham · 2022-11-21T19:27:56Z

I fear you may be right, this change is backwards compatible, but not forwards compatible. Basically what level of support should we provide users who open a project created with CDT 11 in CDT < 11?

jonahgraham · 2022-11-21T19:30:50Z

BTW there is too separate parts of the conversation, changing useByScannerDiscovery default to true is not backwards compatible, so I don't think that can be changed at all.

So now my concern is by introducing ALL_FLAGS we have something that is not forwards compatible from pre-CDT 11 to CDT 11.

I'm going to look at how we handled that kind of not forward compatible change in the past and we'll see what we do from there.

jonahgraham · 2022-11-21T19:49:30Z

On review I don't think we have handled these types of forward compatibility problems properly in the past, so I don't think we need to worry about it now.

For example if in version N a new compiler option is added to the UI, a user checks it, and then opens that project up in version N-1, the option will be silently ignored, which, depending on the option, will lead to compiler error, or differently compiled code.

It would probably be nice to have a warning for projects, similar to what happens when you try to open a workspace in an older version of eclipse, but I think that is outside the scope of this issue.

jonahgraham added the noteworthy Pull requests and fixed issues that should be highlighted to users label Nov 11, 2022

jonahgraham marked this pull request as draft November 11, 2022 03:04

jonahgraham force-pushed the add_ALL_FLAGS_to_scanner_discovery branch 3 times, most recently from e00ad5c to 92abfbf Compare November 11, 2022 03:55

jonahgraham marked this pull request as ready for review November 11, 2022 04:11

jonahgraham requested review from jld01 and Torbjorn-Svensson November 11, 2022 04:11

jonahgraham marked this pull request as draft November 11, 2022 14:18

jonahgraham mentioned this pull request Nov 11, 2022

Various code cleanups #160

Merged

jonahgraham force-pushed the add_ALL_FLAGS_to_scanner_discovery branch 2 times, most recently from 0bcc12c to 9c7a8d7 Compare November 11, 2022 18:03

jonahgraham added this to the 11.0.0 milestone Nov 11, 2022

jonahgraham mentioned this pull request Nov 11, 2022

Use new ALL_FLAGS feature of CDT for discovery eclipse-embed-cdt/eclipse-plugins#547

Draft

Torbjorn-Svensson reviewed Nov 11, 2022

View reviewed changes

jonahgraham force-pushed the add_ALL_FLAGS_to_scanner_discovery branch from 9c7a8d7 to 5b08c5d Compare November 21, 2022 00:45

jonahgraham marked this pull request as ready for review November 21, 2022 00:46

jonahgraham merged commit b8bd158 into eclipse-cdt:main Nov 21, 2022

jonahgraham deleted the add_ALL_FLAGS_to_scanner_discovery branch November 21, 2022 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add all flags to scanner discovery #158

Add all flags to scanner discovery #158

jonahgraham commented Nov 11, 2022 •

edited

Loading

github-actions bot commented Nov 11, 2022 •

edited

Loading

jonahgraham commented Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

jonahgraham commented Nov 12, 2022

Torbjorn-Svensson commented Nov 12, 2022

jonahgraham commented Nov 21, 2022

Torbjorn-Svensson commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

Torbjorn-Svensson commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

Torbjorn-Svensson commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

Add all flags to scanner discovery #158

Add all flags to scanner discovery #158

Conversation

jonahgraham commented Nov 11, 2022 • edited Loading

github-actions bot commented Nov 11, 2022 • edited Loading

Test Results

jonahgraham commented Nov 11, 2022

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

Torbjorn-Svensson Nov 11, 2022

Choose a reason for hiding this comment

jonahgraham commented Nov 12, 2022

Torbjorn-Svensson commented Nov 12, 2022

jonahgraham commented Nov 21, 2022

Torbjorn-Svensson commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

Torbjorn-Svensson commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

Torbjorn-Svensson commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

jonahgraham commented Nov 21, 2022

jonahgraham commented Nov 11, 2022 •

edited

Loading

github-actions bot commented Nov 11, 2022 •

edited

Loading