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.

Sign up for GitHub

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

Jump to bottom

The active Launch Target is now saved in the ICBuildConfiguration #1076

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

The active Launch Target is now saved in the ICBuildConfiguration #1076

wants to merge 2 commits into from
+1,027 −466

Conversation

betamaxbandit
Copy link
Contributor

In addition to the active launch mode and toolchain, the active Launch Target is now used to determine the Core Build Configuration (ICBuildConfiguration).

The build output folder name now ends with the name of the Launch Target (eg: cmake.debug.win32.x86_64.Local)

Addresses Issue: CDT CMake Improvements #1000, IDE-82683-REQ-016 Launch Targets used in ICBuildConfiguration

@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 4, 2025 •
edited
Loading

Test Results

   635 files  +   35     635 suites  +35   35m 43s ⏱️ + 22m 17s
11 427 tests +1 220  11 284 ✅ +1 100  143 💤 +120  0 ❌ ±0 
11 442 runs  +1 197  11 301 ✅ +1 079  141 💤 +118  0 ❌ ±0 

Results for commit 73ea9ae. ± Comparison against base commit f0c74ed.

♻️ This comment has been updated with latest results.

jonahgraham

This comment was marked as resolved.

Sign in to view

@betamaxbandit

This comment was marked as resolved.

Sign in to view
@jonahgraham

This comment was marked as resolved.

Sign in to view
@betamaxbandit

This comment was marked as resolved.

Sign in to view
jonahgraham

This comment was marked as resolved.

Sign in to view

@betamaxbandit

This comment was marked as outdated.

Sign in to view
jonahgraham
jonahgraham reviewed Feb 6, 2025
View reviewed changes
cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java Outdated Show resolved Hide resolved
cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java Outdated Show resolved Hide resolved
cmake/org.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfiguration.java Outdated Show resolved Hide resolved
...g.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfigurationProvider.java Outdated Show resolved Hide resolved
...g.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfigurationProvider.java Outdated Show resolved Hide resolved
...g.eclipse.cdt.cmake.core/src/org/eclipse/cdt/cmake/core/CMakeBuildConfigurationProvider.java Outdated Show resolved Hide resolved
core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/CCorePlugin.java Show resolved Hide resolved
core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/CBuildConfiguration.java Outdated Show resolved Hide resolved
...org.eclipse.cdt.core/src/org/eclipse/cdt/internal/core/build/CBuildConfigurationManager.java Show resolved Hide resolved
....eclipse.cdt.debug.core/src/org/eclipse/cdt/debug/core/launch/CoreBuildLaunchBarTracker.java Show resolved Hide resolved
@jonahgraham jonahgraham mentioned this pull request Feb 6, 2025
Open
jonahgraham

This comment was marked as resolved.

Sign in to view

betamaxbandit

This comment was marked as outdated.

Sign in to view

@betamaxbandit
The active Launch Target is now saved in the ICBuildConfiguration
c718fb0 
In addition to the active launch mode and toolchain, the active Launch
Target is now used to determine the Core Build Configuration
(ICBuildConfiguration).

The build output folder name now ends with the name of the Launch Target
(eg: cmake.debug.win32.x86_64.Local)

Additionally, API added to allow the ISV to configure the build output
folder name.

The ICBuildConfigurationProvider implementations, shown below, have been
tidied up to use common code.

  CMakeBuildConfigurationProvider
  AutotoolsBuildConfigurationProvider
  MakefileBuildConfigurationProvider
  MesonBuildConfigurationProvider

Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-016 Launch
Targets used in ICBuildConfiguration
Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-018 Build
output folder name
@betamaxbandit

This comment was marked as resolved.

Sign in to view
@betamaxbandit

This comment was marked as resolved.

Sign in to view
@betamaxbandit
Copy link
Contributor Author

@jonahgraham ,

I see the "Code Cleanliness Checks / build (pull_request) Failing after 3m" job failed for my PR, but in the detail I don't see what the problem is. Can you help please?

@betamaxbandit betamaxbandit marked this pull request as ready for review February 11, 2025 21:52
jonahgraham
jonahgraham reviewed Feb 12, 2025
View reviewed changes
Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@betamaxbandit There are a few small changes which I will apply manually. Please have a look and let me know if they are ok. And if so I will merge them

I did test autotools and it looks fine.

NewAndNoteworthy/CHANGELOG-API.md Outdated Show resolved Hide resolved
...eclipse.cdt.core.tests/model/org/eclipse/cdt/core/build/CBuildConfigurationManagerTests.java Outdated Show resolved Hide resolved
...eclipse.cdt.core.tests/model/org/eclipse/cdt/core/build/CBuildConfigurationManagerTests.java Outdated Show resolved Hide resolved
...lipse.cdt.core.tests/model/org/eclipse/cdt/core/build/ICBuildConfigurationProviderTests.java Outdated
/*
* Note, this extra test uses getName on CBuildConfiguration rather than the API ICBuildConfiguration
*/
String name = cbc.getName();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the place you were mentioning that you wanted me to check about use of non-API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's it

...lipse.cdt.core.tests/model/org/eclipse/cdt/core/build/ICBuildConfigurationProviderTests.java Outdated Show resolved Hide resolved
core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/ICBuildConfigurationManager.java Outdated Show resolved Hide resolved
@jonahgraham
Copy link
Member

@betamaxbandit here is a link to to my application of the changes in the review tab. It can also be seen by clicking on the commit in the timeline above:

image

betamaxbandit
betamaxbandit commented Feb 12, 2025
View reviewed changes
Copy link
Contributor Author

@betamaxbandit betamaxbandit left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Hi @jonahgraham ,
thank you for all your suggestions. I agree with all of them.
I see "Apply review comments", but can't click it.
Please go ahead and commit these if that is what you meant.
Thanks John

@betamaxbandit
Copy link
Contributor Author

@betamaxbandit There are a few small changes which I will apply manually. Please have a look and let me know if they are ok. And if so I will merge them

I did test autotools and it looks fine.

Great, thank you. I'm happy with all the changes you suggest.
Thanks for testing autotools.

@jonahgraham jonahgraham added the noteworthy Pull requests and fixed issues that should be highlighted to users label Feb 12, 2025
@jonahgraham jonahgraham added this to the 12.0.0 M3 milestone Feb 12, 2025
@jonahgraham
Copy link
Member

@betamaxbandit I think this is ready to squash and merge. Let me know when you are done your final check.

@betamaxbandit
Copy link
Contributor Author

@betamaxbandit I think this is ready to squash and merge. Let me know when you are done your final check.

Hi @jonahgraham ,
I've tested the latest commit and all looks fine. Please go ahead and merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonahgraham jonahgraham jonahgraham left review comments

Assignees
No one assigned
Labels
noteworthy Pull requests and fixed issues that should be highlighted to users
Projects
None yet
Milestone
12.0.0 M3
Development

Successfully merging this pull request may close these issues.
2 participants
@betamaxbandit @jonahgraham