Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use new ALL_FLAGS feature of CDT for discovery #547

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jonahgraham
Copy link
Contributor

CDT 11 includes a new ${ALL_FLAGS} that can be specified in the scanner discovery to pick up all command line options specified in the managed build.

See the N&N entry at:

https://github.com/jonah/cdt/blob/main/NewAndNoteworthy/CDT-11.0.md#scanner-discovery-considering-all-flags
(here until eclipse-cdt/cdt#158 is merged)

This change is not required as CDT's change is non-breaking. But to use ALL_FLAGS you need CDT 11 or newer, specifically for org.eclipse.cdt.managedbuilder.core;bundle-version="9.5.0"

CDT 11 includes a new ${ALL_FLAGS} that can be specified in the
scanner discovery to pick up all command line options specified
in the managed build.

See the N&N entry at:

https://github.com/jonah/cdt/blob/main/NewAndNoteworthy/CDT-11.0.md#scanner-discovery-considering-all-flags

This change is not required as CDT's change is non-breaking. But
to use ALL_FLAGS you need CDT 11 or newer, specifically for
org.eclipse.cdt.managedbuilder.core;bundle-version="9.5.0"
@jonahgraham jonahgraham marked this pull request as draft November 11, 2022 18:12
@jonahgraham
Copy link
Contributor Author

Marked as draft until eclipse-cdt/cdt#158 is resolved.

@ilg-ul
Copy link
Contributor

ilg-ul commented Nov 11, 2022

I'm not sure I understand the details. :-(

Anyway, embed-cdt is based on an older CDT, and I am reluctant to update to CDT 11.

@jonahgraham
Copy link
Contributor Author

I'm not sure I understand the details. :-(

Another concrete example is in the original bug report, if a user specifies -mcpu=cortex-m4 then the __ARM_ARCH pre-defined macro will be 7, but CDT won't know that because it didn't pass the -mcpu=cortex-m4` to GCC when querying GCC for the pre-defined macros. As a result the indexing of source files will be incorrect.

With CDT 10, you can fix the above situation (if you agree it is a problem) by adding the useByScannerDiscovery="true" to the -mcpu= option in the plugin.xml, like you have done for other options.

What this change does is effectively defaults to include all options for scanner discovery, except those explictly excluded.

I am reluctant to update to CDT 11.

That's fine - if users are using CDT 11, like with Eclipse 2022-12 they can make the change manually. This commit only changes the default for new projects and on its own is certainly not enough to bother updating to CDT 11.

@ilg-ul
Copy link
Contributor

ilg-ul commented Nov 12, 2022

As things are now, the update URL is https://download.eclipse.org/embed-cdt/updates/v6 and is expected to be valid for existing Eclipses back to the initial Eclipse used after the migration to EF, so as long as these old Eclipses can still be updated, we should be fine.

If, for any reason, we decide to bump the base version to a more recent Eclipse/CDT, we should increase the version to v7 and change the update URL. These changes take some time, thus my reluctance, but otherwise any improvements are welcome.

@jonahgraham
Copy link
Contributor Author

We can leave this as a draft until then, whenever "then" is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants