Bug 580015: add support for multiple bin parsers #75

betamaxbandit · 2022-09-26T19:42:48Z

Added ability to return a list of binary parser IDs, rather than a single ID. This supports build configurations that have multiple binaries with for example cross toolchains.

Change-Id: I1b7e47bf6a86bbd9f1c6b9646d008bac9479417d

Added ability to return a list of binary parser IDs, rather than a single ID. This supports build configurations that have multiple binaries with for example cross toolchains. Change-Id: I1b7e47bf6a86bbd9f1c6b9646d008bac9479417d

jonahgraham

This is migrated from https://git.eclipse.org/r/c/cdt/org.eclipse.cdt/+/194075 - so the comments from it are migrated to it.

core/org.eclipse.cdt.core/.settings/.api_filters

core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/ICBuildConfiguration.java

Update since tags to 8.0. Remove api filter. Fix other since tags after removing of api filter. Remove interface defaults. Add default implementations where necessary. Update tests - TBC.

betamaxbandit · 2022-09-28T22:24:59Z

Hi @jonahgraham ,
Can you review my changes to the actual code please? Note I'm still working on the tests (TestICBuildConfiguration), but ran out of time today.

jonahgraham

I didn't look at tests, other than the location of them. Let me know if you need a suggestion of where they should go. It may also be appropriate to leave them there if there isn't anywhere better already.

core/org.eclipse.cdt.core.tests/META-INF/MANIFEST.MF

core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/ICBuildConfiguration.java

core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/IToolChain.java

jonahgraham · 2022-11-07T22:38:44Z

@betamaxbandit Will you have time to look at this this week? It would be ideal to get this done in time for M3 which is 14 Nov.

betamaxbandit · 2022-11-07T23:04:03Z

@betamaxbandit Will you have time to look at this this week? It would be ideal to get this done in time for M3 which is 14 Nov.

Hi Jonah,

Yes, I'll try. Thanks for the reminder.

github-actions · 2022-11-14T19:56:02Z

Test Results

    593 files     593 suites 16m 11s ⏱️
10 126 tests 10 104 ✔️ 22 💤 0 ❌
10 142 runs 10 120 ✔️ 22 💤 0 ❌

Results for commit 99a2295.

♻️ This comment has been updated with latest results.

jonahgraham · 2022-11-14T20:02:33Z

There is a compilation failure now BTW:

Error:  Failed to execute goal org.eclipse.tycho:tycho-compiler-plugin:2.7.5:compile (default-compile) on project org.eclipse.cdt.docker.launcher: Compilation failure: Compilation failure: 
Error:  /home/runner/work/cdt/cdt/launch/org.eclipse.cdt.docker.launcher/src/org/eclipse/cdt/internal/docker/launcher/ui/launchbar/ContainerGCCToolChain.java:[70] 
Error:  	public class ContainerGCCToolChain extends PlatformObject implements IToolChain, IToolChain2 {
Error:  	             ^^^^^^^^^^^^^^^^^^^^^
Error:  The type ContainerGCCToolChain must implement the inherited abstract method IToolChain.getBinaryParserIds()
Error: [ERROR] 1 problem (1 error)
Error:  -> [Help 1]

betamaxbandit · 2022-11-14T20:17:02Z

Hi @jonahgraham ,
I didn't mean to do that. Still getting used to the github i/f. I was trying to rebase on main, but not sure I did it correctly.

Removed ICBuildConfiguration.getBinaryParserId() and IToolChain.getBinaryParserId(). Replaced with methods that return a list of IDs. Updated API changes doc. Rearranged tests so that the test for IToolChain is in a new gcc test plugin.

Added remaining project metadata for new gcc test plugin.

Fixed build props of new test plugin. Fixed test, cleanup project afterwards.

jonahgraham · 2022-11-15T14:06:02Z

Don't worry about the code cleanliness checks fails - there is a proposal to automate this in #166

betamaxbandit · 2022-11-15T14:16:01Z

Don't worry about the code cleanliness checks fails - there is a proposal to automate this in #166

Hi Jonah, ok, I won't.
I fixed the remaining problem which was the junit tests were failing. All tests pass now :-)
BTW thanks for bringing the "draft"/WIP status thing to my attention. That's useful when you're trying to finesse something.
Ready for review now.

jonahgraham

This looks fine, just needs a N&N entry about the improved API.

In https://github.com/eclipse-cdt/cdt/blob/main/NewAndNoteworthy/CDT-11.0.md below Binary Parser code uses AutoCloseable section

## Core Build tool chains can now return multiple binary parsers

The `ICBuildConfiguration` and `IToolChain` interfaces now have a method, `getBinaryParserIds` that allows a build configuration or tool chain to return multiple binary parsers.

See https://github.com/eclipse-cdt/cdt/pull/75

NewAndNoteworthy/CHANGELOG-API.md

jonahgraham · 2022-11-15T14:50:03Z

Please mark this as ready for review when you are happy with it.

Update API and News documentation.

betamaxbandit · 2022-11-15T15:10:36Z

Please mark this as ready for review when you are happy with it.

Ready for review.

jonahgraham

LGTM - needs the code cleanliness sorted out. I would like to use this PR to test #166 if I can get that done.

betamaxbandit · 2022-11-15T15:31:09Z

LGTM - needs the code cleanliness sorted out. I would like to use this PR to test #166 if I can get that done.

Oh ok. I don't really understand the actual cleanliness error. It says the following files are missing, but those file don't exist for me locally - only org.eclipse.jdt.core.prefs. Am I expected to generate them?

build/org.eclipse.cdt.build.gcc.core.tests/.settings/org.eclipse.core.resources.prefs
build/org.eclipse.cdt.build.gcc.core.tests/.settings/org.eclipse.jdt.launching.prefs
build/org.eclipse.cdt.build.gcc.core.tests/.settings/org.eclipse.jdt.ui.prefs
build/org.eclipse.cdt.build.gcc.core.tests/.settings/org.eclipse.pde.api.tools.prefs
build/org.eclipse.cdt.build.gcc.core.tests/.settings/org.eclipse.pde.prefs

Do you want me to do anything for #166 ?

jonahgraham · 2022-11-15T17:32:41Z

Do you want me to do anything for #166 ?

No.

If I can get #166 to work it will magically and automatically fix this PR. If not I will fix it up manually and merge it in a few days (ahead of RC1 on Monday)

Removed ICBuildConfiguration.getBinaryParserId() and IToolChain.getBinaryParserId(). Replaced with methods that return a list of IDs. Updated API changes doc. Rearranged tests so that the test for IToolChain is in a new gcc test plugin.

Added remaining project metadata for new gcc test plugin.

Fixed build props of new test plugin. Fixed test, cleanup project afterwards.

Update API and News documentation.

jonahgraham · 2022-11-18T15:57:46Z

I discussed with @betamaxbandit and we agreed to decouple this from #166. So I manually fixed up the code cleanliness issue and pushed it.

PS I accidentally "rebased and merged" instead of "squashed and merged" so I ended up with a dumb commit message c4761cf - sorry about that.

betamaxbandit · 2022-11-18T16:22:13Z

Thanks for merging @jonahgraham

betamaxbandit added 2 commits September 26, 2022 20:30

Bug 580015: add support for multiple bin parsers

885d950

Added ability to return a list of binary parser IDs, rather than a single ID. This supports build configurations that have multiple binaries with for example cross toolchains. Change-Id: I1b7e47bf6a86bbd9f1c6b9646d008bac9479417d

Bump version number ahead of breaking API change.

7bfc423

jonahgraham requested changes Sep 26, 2022

View reviewed changes

core/org.eclipse.cdt.core/.settings/.api_filters Outdated Show resolved Hide resolved

core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/ICBuildConfiguration.java Outdated Show resolved Hide resolved

betamaxbandit added 2 commits September 28, 2022 21:23

Adjusting version range in dependent plugins

d66f966

Update for major version bump

e06a84b

Update since tags to 8.0. Remove api filter. Fix other since tags after removing of api filter. Remove interface defaults. Add default implementations where necessary. Update tests - TBC.

jonahgraham requested changes Sep 29, 2022

View reviewed changes

jonahgraham mentioned this pull request Nov 1, 2022

Bug 580966: Core Build Makefile, option to change toolchain per proce… #130

Closed

Merge branch 'eclipse-cdt:main' into multiBinParse

a17a595

betamaxbandit added 3 commits November 14, 2022 23:18

Merge branch 'eclipse-cdt:main' into multiBinParse

c0e5ad1

Bug 580015: add support for multiple bin parsers eclipse-cdt#75

febb6ef

Added remaining project metadata for new gcc test plugin.

betamaxbandit marked this pull request as draft November 15, 2022 12:29

Bug 580015: add support for multiple bin parsers eclipse-cdt#75

04ac996

Fixed build props of new test plugin. Fixed test, cleanup project afterwards.

betamaxbandit marked this pull request as ready for review November 15, 2022 14:14

jonahgraham requested changes Nov 15, 2022

View reviewed changes

NewAndNoteworthy/CHANGELOG-API.md Outdated Show resolved Hide resolved

Bug 580015: add support for multiple bin parsers eclipse-cdt#75

6a43cb6

Update API and News documentation.

betamaxbandit requested a review from jonahgraham November 15, 2022 15:11

jonahgraham requested changes Nov 15, 2022

View reviewed changes

fix code cleanlines

99a2295

jonahgraham approved these changes Nov 18, 2022

View reviewed changes

jonahgraham merged commit c4761cf into eclipse-cdt:main Nov 18, 2022

jonahgraham pushed a commit that referenced this pull request Nov 18, 2022

Bug 580015: add support for multiple bin parsers #75

fc821a4

Added remaining project metadata for new gcc test plugin.

jonahgraham pushed a commit that referenced this pull request Nov 18, 2022

Bug 580015: add support for multiple bin parsers #75

c4c6345

Fixed build props of new test plugin. Fixed test, cleanup project afterwards.

jonahgraham pushed a commit that referenced this pull request Nov 18, 2022

Bug 580015: add support for multiple bin parsers #75

4877f52

Update API and News documentation.

betamaxbandit deleted the multiBinParse branch November 20, 2023 17:21

jonahgraham mentioned this pull request Feb 10, 2024

[703] fix NPE when no build binaries on system PATH #704

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 580015: add support for multiple bin parsers #75

Bug 580015: add support for multiple bin parsers #75

betamaxbandit commented Sep 26, 2022

jonahgraham left a comment

betamaxbandit commented Sep 28, 2022

jonahgraham left a comment

jonahgraham commented Nov 7, 2022

betamaxbandit commented Nov 7, 2022

github-actions bot commented Nov 14, 2022 •

edited

Loading

jonahgraham commented Nov 14, 2022

betamaxbandit commented Nov 14, 2022

jonahgraham commented Nov 15, 2022

betamaxbandit commented Nov 15, 2022 •

edited

Loading

jonahgraham left a comment

jonahgraham commented Nov 15, 2022

betamaxbandit commented Nov 15, 2022

jonahgraham left a comment

betamaxbandit commented Nov 15, 2022

jonahgraham commented Nov 15, 2022

jonahgraham commented Nov 18, 2022

betamaxbandit commented Nov 18, 2022

Bug 580015: add support for multiple bin parsers #75

Bug 580015: add support for multiple bin parsers #75

Conversation

betamaxbandit commented Sep 26, 2022

jonahgraham left a comment

Choose a reason for hiding this comment

betamaxbandit commented Sep 28, 2022

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham commented Nov 7, 2022

betamaxbandit commented Nov 7, 2022

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

Test Results

jonahgraham commented Nov 14, 2022

betamaxbandit commented Nov 14, 2022

jonahgraham commented Nov 15, 2022

betamaxbandit commented Nov 15, 2022 • edited Loading

jonahgraham left a comment

Choose a reason for hiding this comment

jonahgraham commented Nov 15, 2022

betamaxbandit commented Nov 15, 2022

jonahgraham left a comment

Choose a reason for hiding this comment

betamaxbandit commented Nov 15, 2022

jonahgraham commented Nov 15, 2022

jonahgraham commented Nov 18, 2022

betamaxbandit commented Nov 18, 2022

github-actions bot commented Nov 14, 2022 •

edited

Loading

betamaxbandit commented Nov 15, 2022 •

edited

Loading