The active Launch Target is now saved in the ICBuildConfiguration #1076
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
betamaxbandit
force-pushed
the
IDE-82683-REQ-016
branch
from
22f1648 to
382ce1e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
betamaxbandit
force-pushed
the
IDE-82683-REQ-016
branch
from
382ce1e to
a98d721
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
betamaxbandit
force-pushed
the
IDE-82683-REQ-016
branch
from
a98d721 to
4871559
Compare
This comment was marked as outdated.
This comment was marked as outdated.
jonahgraham
reviewed
Feb 6, 2025
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/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
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
betamaxbandit
force-pushed
the
IDE-82683-REQ-016
branch
from
4871559 to
23f9e03
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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
force-pushed
the
IDE-82683-REQ-016
branch
from
23f9e03 to
c718fb0
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
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? |
jonahgraham
reviewed
Feb 12, 2025
There was a problem hiding this comment.
@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.
...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
| /* | ||
| * Note, this extra test uses getName on CBuildConfiguration rather than the API ICBuildConfiguration | ||
| */ | ||
| String name = cbc.getName(); |
There was a problem hiding this comment.
Is this the place you were mentioning that you wanted me to check about use of non-API?
There was a problem hiding this comment.
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
|
@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:
|
betamaxbandit
commented
Feb 12, 2025
There was a problem hiding this comment.
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
Great, thank you. I'm happy with all the changes you suggest. |
jonahgraham
added
the
noteworthy
|
@betamaxbandit I think this is ready to squash and merge. Let me know when you are done your final check. |
Hi @jonahgraham , |
|
Thanks @betamaxbandit for this significant contribution. |
betamaxbandit
added a commit
to betamaxbandit/cdt
that referenced
this pull request
Feb 13, 2025
The default IBuildConfiguration is no longer used by projects that use ICBuildConfigurationProvider. For CMake, Makefile and other projects the build output folder is sometimes named "default" rather than the pattern toolName.launchMode.toolchain OS.toolchain Arch.launchTarget Id (eg: cmake.debug.win32.x86_64.Local). PR eclipse-cdt#1076 exposes new API (ICBuildConfigurationProvider.getCBuildConfigName) to encourage this naming pattern. The "sometimes" is variable and often happens when a project is first created when the active launch target is Local and the launch mode is "run", but not always. This gives a random, inconsistent impression to CDT. The Platform project always contains a IBuildConfiguration with the name IBuildConfiguration.DEFAULT_CONFIG_NAME. It seems the original Core Build system design went to some length to fit in with this and always make use of this IBuildConfiguration when pairing it with a new ICBuildConfiguration. With this PR, this no longer happens, allowing CDT code to be simplified and the build folder naming made consistent, always adhering to ICBuildConfigurationProvider.getCBuildConfigName. Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-024 Default CMake build folder
betamaxbandit
added a commit
to betamaxbandit/cdt
that referenced
this pull request
Feb 18, 2025
The default IBuildConfiguration is no longer used by projects that use ICBuildConfigurationProvider. For CMake, Makefile and other Core Build projects the build output folder is sometimes named "default" rather than the pattern toolName.launchMode.toolchain OS.toolchain Arch.launchTarget Id (eg: cmake.debug.win32.x86_64.Local). PR eclipse-cdt#1076 exposes new API (ICBuildConfigurationProvider.getCBuildConfigName) to encourage this naming pattern. The "sometimes" is variable and often happens when a project is first created when the active launch target is Local and the launch mode is "run", but not always. This gives a random, inconsistent impression to CDT. The Platform project always contains a IBuildConfiguration with the name IBuildConfiguration.DEFAULT_CONFIG_NAME. It seems the original Core Build system design went to some length to fit in with this and always make use of this IBuildConfiguration when pairing it with a new ICBuildConfiguration. With this PR, this no longer happens, allowing CDT code to be simplified and the build folder naming made consistent, always adhering to ICBuildConfigurationProvider.getCBuildConfigName. Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-024 Default CMake build folder
betamaxbandit
added a commit
to betamaxbandit/cdt
that referenced
this pull request
Feb 18, 2025
The default IBuildConfiguration is no longer used by projects that use ICBuildConfigurationProvider. For CMake, Makefile and other Core Build projects the build output folder is sometimes named "default" rather than the pattern toolName.launchMode.toolchain OS.toolchain Arch.launchTarget Id (eg: cmake.debug.win32.x86_64.Local). PR eclipse-cdt#1076 exposes new API (ICBuildConfigurationProvider.getCBuildConfigName) to encourage this naming pattern. The "sometimes" is variable and often happens when a project is first created when the active launch target is Local and the launch mode is "run", but not always. This gives a random, inconsistent impression to CDT. The Platform project always contains a IBuildConfiguration with the name IBuildConfiguration.DEFAULT_CONFIG_NAME. It seems the original Core Build system design went to some length to fit in with this and always make use of this IBuildConfiguration when pairing it with a new ICBuildConfiguration. With this PR, this no longer happens, allowing CDT code to be simplified and the build folder naming made consistent, always adhering to ICBuildConfigurationProvider.getCBuildConfigName. Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-024 Default CMake build folder
betamaxbandit
added a commit
to betamaxbandit/cdt
that referenced
this pull request
Feb 19, 2025
The default IBuildConfiguration is no longer used by projects that use ICBuildConfigurationProvider. For CMake, Makefile and other Core Build projects the build output folder is sometimes named "default" rather than the pattern toolName.launchMode.toolchain OS.toolchain Arch.launchTarget Id (eg: cmake.debug.win32.x86_64.Local). PR eclipse-cdt#1076 exposes new API (ICBuildConfigurationProvider.getCBuildConfigName) to encourage this naming pattern. The "sometimes" is variable and often happens when a project is first created when the active launch target is Local and the launch mode is "run", but not always. This gives a random, inconsistent impression to CDT. The Platform project always contains a IBuildConfiguration with the name IBuildConfiguration.DEFAULT_CONFIG_NAME. It seems the original Core Build system design went to some length to fit in with this and always make use of this IBuildConfiguration when pairing it with a new ICBuildConfiguration. With this PR, this no longer happens, allowing CDT code to be simplified and the build folder naming made consistent, always adhering to ICBuildConfigurationProvider.getCBuildConfigName. Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-024 Default CMake build folder
jonahgraham
pushed a commit
to betamaxbandit/cdt
that referenced
this pull request
Feb 20, 2025
The default IBuildConfiguration is no longer used by projects that use ICBuildConfigurationProvider. For CMake, Makefile and other Core Build projects the build output folder is sometimes named "default" rather than the pattern toolName.launchMode.toolchain OS.toolchain Arch.launchTarget Id (eg: cmake.debug.win32.x86_64.Local). PR eclipse-cdt#1076 exposes new API (ICBuildConfigurationProvider.getCBuildConfigName) to encourage this naming pattern. The "sometimes" is variable and often happens when a project is first created when the active launch target is Local and the launch mode is "run", but not always. This gives a random, inconsistent impression to CDT. The Platform project always contains a IBuildConfiguration with the name IBuildConfiguration.DEFAULT_CONFIG_NAME. It seems the original Core Build system design went to some length to fit in with this and always make use of this IBuildConfiguration when pairing it with a new ICBuildConfiguration. With this PR, this no longer happens, allowing CDT code to be simplified and the build folder naming made consistent, always adhering to ICBuildConfigurationProvider.getCBuildConfigName. Addresses Issue: CDT CMake Improvements eclipse-cdt#1000, IDE-82683-REQ-024 Default CMake build folder
jonahgraham
pushed a commit
that referenced
this pull request
Feb 20, 2025
The default IBuildConfiguration is no longer used by projects that use ICBuildConfigurationProvider. For CMake, Makefile and other Core Build projects the build output folder is sometimes named "default" rather than the pattern toolName.launchMode.toolchain OS.toolchain Arch.launchTarget Id (eg: cmake.debug.win32.x86_64.Local). PR #1076 exposes new API (ICBuildConfigurationProvider.getCBuildConfigName) to encourage this naming pattern. The "sometimes" is variable and often happens when a project is first created when the active launch target is Local and the launch mode is "run", but not always. This gives a random, inconsistent impression to CDT. The Platform project always contains a IBuildConfiguration with the name IBuildConfiguration.DEFAULT_CONFIG_NAME. It seems the original Core Build system design went to some length to fit in with this and always make use of this IBuildConfiguration when pairing it with a new ICBuildConfiguration. With this PR, this no longer happens, allowing CDT code to be simplified and the build folder naming made consistent, always adhering to ICBuildConfigurationProvider.getCBuildConfigName. Addresses Issue: CDT CMake Improvements #1000, IDE-82683-REQ-024 Default CMake build folder
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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