Skip to content

Fix for the compiler path in compile_commands.json file #1138

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
Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix for the compiler path in compile_commands.json file #1138

wants to merge 2 commits into from

Conversation

alicetrifu
Copy link
Contributor

Compiler path was wrongly created when anything was set on the "Prefix" option from "Cross Settings". The code needs to check if anything is set there before creating the default path for the compiler.

@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 10, 2025 •
edited
Loading

Test Results

   602 files     602 suites   13m 0s ⏱️
10 224 tests 10 199 ✅ 24 💤 1 ❌
10 246 runs  10 221 ✅ 24 💤 1 ❌

For more details on these failures, see this check.

Results for commit cfea01e.

♻️ This comment has been updated with latest results.

@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from 1151c1e to dabaf25 Compare April 10, 2025 14:34
@jonahgraham jonahgraham linked an issue Apr 10, 2025 that may be closed by this pull request
CDT fails to generate correct commands in compile_commands.json file #1130
Open
@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from dabaf25 to 080da79 Compare April 10, 2025 15:11
jonahgraham
jonahgraham requested changes Apr 10, 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.

This looks like roughly the correct solution. Just some cleanup and review the caching.

@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from 080da79 to d448a14 Compare April 10, 2025 15:57
jonahgraham
jonahgraham reviewed Apr 10, 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.

This works well in the normal cases, but there are some corner cases that aren't handled well, see the individual comments.

@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from d448a14 to 2c4aa3f Compare April 11, 2025 07:41
jonahgraham
jonahgraham approved these changes Apr 23, 2025
View reviewed changes
...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java Outdated
if (arguments.length <= 1) {
return ""; //$NON-NLS-1$
}
return String.join(" ", Arrays.copyOfRange(arguments, 1, arguments.length)); //$NON-NLS-1$
Copy link
Member

Choose a reason for hiding this comment

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

This is an invalid way of converting a list to a command line string. For example gcc "My file.c" -o "My file.o" would turn into "/usr/bin/gcc" My file.c -o My file.o which won't be interpreted properly. Please ensure quoting is done as appropriate and meets the spec of compile_commands.json linked here

org.eclipse.cdt.utils.CommandLineUtil.argumentsToString(Collection, boolean) may or may not actually match the requirements. I hope that it does as it would be simpler than implementing a new case. If argumentsToString does not meet requirements, I recommend adding a new method to CommandLineUtil and the appropriate tests to CommandLineUtilTest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @jonahgraham . Thank you for the review. I Think we need to fix this by implementing indeed a new case. I did this and also added a JUnit test especially for this so we can check this properly.

Please let me know if you think we need to do any other adjustments.

Thank you!

...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java Outdated
private String findCompilerInPath(ITool tool, IConfiguration config) {
if (toolMap.containsKey(tool)) {
return "\"" + toolMap.get(tool) + "\""; //$NON-NLS-1$//$NON-NLS-2$
private String findCompilerInPath(ITool tool, IConfiguration config, String compilerName) {
Copy link
Member

Choose a reason for hiding this comment

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

tool is now unused.

Suggested change
private String findCompilerInPath(ITool tool, IConfiguration config, String compilerName) {
private String findCompilerInPath(IConfiguration config, String compilerName) {

...eclipse/cdt/managedbuilder/internal/core/jsoncdb/generator/CompilationDatabaseGenerator.java
@@ -70,7 +71,7 @@ public final class CompilationDatabaseGenerator {
* Checked on each build
* Used before we look up the environment
*/
private Map<ITool, String> toolMap = new HashMap<>();
private Map<String, String> toolMap = new HashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

Because we now have a key of a string it is much less obvious what that key is, so please document it. Perhaps a line like this in the javadoc: Map of compiler name (as calculated by {@link #getCompilerName(String)}) to the absolute path of the compiler.

@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from 2c4aa3f to 84316a4 Compare April 24, 2025 09:37
@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from 84316a4 to d01e65c Compare May 7, 2025 12:03
@alicetrifu
Fix for the compiler path in compile_commands.json file
70bd427 
Compiler path was wrongly created when anything was set on the "Prefix"
option from "Cross Settings". The code needs to check if anything is set
there before creating the default path for the compiler.
@alicetrifu alicetrifu force-pushed the fixPathForCompiler1130 branch from d01e65c to 70bd427 Compare May 7, 2025 12:57
@ghentschke ghentschke mentioned this pull request Sep 4, 2025
Open
@ghentschke
Copy link
Contributor

@alicetrifu Is this PR still work in progress?

@alicetrifu
Copy link
Contributor Author

Hello @ghentschke ,

This should be done. I see that the logs are not available anymore for this, but the comments should be fixed. I see that meanwhile this got into conflict. Should I fix this?

@ghentschke
Copy link
Contributor

ghentschke commented Sep 4, 2025 •
edited
Loading

Should I fix this?

That would be great. I think we should fix this, since another user has stumbled around this issue. See eclipse-cdt/cdt-lsp#531

but the comments should be fixed.

Yes

@alicetrifu
Copy link
Contributor Author

@ghentschke, I have fixed the conflict directly form the Github UI. I hope this is fine. Will wait for the final result of the checks. Thank you!

@ghentschke
Copy link
Contributor

ghentschke commented Sep 5, 2025 •
edited
Loading

There is still a failed unit test. I am not sure if it relates to this PR.
I don't think so, as other lately build PR have same broken unit test.

@alicetrifu
Copy link
Contributor Author

@ghentschke , This does not seem to be related to my PR indeed. But I am not sure why I have the problem with the Licence. I had this in the past as well and @Kummallinen rerun this for me. I think there are some issues with my permissions. Any idea on what I should do next to fix this ? Thank you!

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

@jonahgraham jonahgraham jonahgraham approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CDT fails to generate correct commands in compile_commands.json file
3 participants
@alicetrifu @ghentschke @jonahgraham