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

[ewaterlander]

…ssed console line.

Classes that extend the Core Build Makefile project (StandardBuildConfiguration) can change the tool chain for every source file.

[jonahgraham]

I am concerned about this change because in the past there was a get/set for toolchain that was explicitly removed in https://git.eclipse.org/r/c/cdt/org.eclipse.cdt/+/72439/3/core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/CBuildConfiguration.java#192

I fear I don't understand this code base well enough to know if what the issue you have falls outside the original design, or if there is a different way to do what you are trying to do.

I also don't understand your use case that well either. Why do you want to change the toolchain on each line of output?

[jonahgraham]

I don't think this should be new API here. Feel free to add a new method to CommandLineUtil though, perhaps called argumentsToList?

[ewaterlander]

OK.

[jonahgraham]

By adding a set and changing the code to call get everywhere it implies that there is now two different ways to change the active toolchain:

1. Calling setToolChain
2. Overridding getToolChain

If getToolChain must be called to get the toolchain, then probably should document that on private IToolChain toolChain - but ideally I think just adding the setToolChain and make toolChain non-final.

That would remove all the other changes here.

[ewaterlander]

The reason for my change is that I need support for multiple C compilers in one Makefile. The core build CMake flow also can do it. Only there is a cmake limitation to support only one compiler in one run.
We have discussed this in the cdt-dev mailing list "Managing include paths and macros in 'Core Build' projects." https://www.eclipse.org/lists/cdt-dev/threads.html#35316

I started with overriding getToolChain, and putting getToolChain everywhere. And this worked for me. At the end I added setToolChain, because I thought if anyone adds code that directly uses the toolChain variable my plugin will be broken.

If you are fine with adding setToolChain(), I will remove the other changes.

[jonahgraham]

I want to loop in @betamaxbandit because I think he has been trying to resolve a similar issue - see PR #75

IIUC @betamaxbandit is expanding IToolChain (and others) to support a single "tool chain" that is actually multiple compilers.

This sounds like the same thing here.

When IToolChain.getResourcesFromCommand is called, then the tool chain should delegate to the right "sub-tool chain" (something I just made up). Maybe to make @betamaxbandit and your issue easier we need a "composite" tool chain that understands how to delegate properly. Perhaps something like:

/**
 * @since 8.0
 */
public abstract class CompositeToolChain implements IToolChain {

	private List<IToolChain> children = new ArrayList<>();
	private IToolChainProvider provider;

	public CompositeToolChain(IToolChainProvider provider, List<IToolChain> children) {
		this.provider = provider;
		this.children.addAll(children);
	}

	@Override
	public IToolChainProvider getProvider() {
		return provider;
	}

	@Override
	public List<String> getBinaryParserIds() {
		return children.stream().flatMap(c -> c.getBinaryParserIds().stream()).collect(Collectors.toUnmodifiableList());
	}

	@Override
	public Path getCommandPath(Path command) {
		return children.stream().map(c -> c.getCommandPath(command)).filter(Objects::nonNull).findFirst().orElse(null);
	}

	@Override
	public String[] getCompileCommands() {
		return children.stream().flatMap(c -> Arrays.stream(c.getCompileCommands())).toArray(String[]::new);
	}

	@Override
	public IResource[] getResourcesFromCommand(List<String> command, URI buildDirectoryURI) {
		return children.stream().flatMap(c -> Arrays.stream(c.getResourcesFromCommand(command, buildDirectoryURI)))
				.toArray(IResource[]::new);
	}
}

To better understand the problem, what is the logic that your sub-class of CBuildConfiguration is using to change the tool chain? Can that logic exist in a composite tool chain as I described above?

[ewaterlander]

This is a case where the default compiler is gcc, and based on the command line I can change it to "myCompiler" using "myToolChain". I did this by overriding getToolChain(). Per processed line I set myToolChain.

    @Override
    public IToolChain getToolChain() throws CoreException {
        if (myToolChain == null) {
            return super.getToolChain();
        } else {
            return myToolChain;
        }
    }

    private IToolChain getToolChain(List<String> command) throws CoreException {
        if (command.size() > 0 && command.get(0).endsWith("/myCompiler")) {
            IToolChainManager toolChainManager = CCorePlugin.getService(IToolChainManager.class);
            Map<String, String> properties = new HashMap<String, String>() {
                private static final long serialVersionUID = 4493246628665145757L;

                {
                    put(IToolChain.ATTR_ARCH, "myArch");
                }
            };
            Collection<IToolChain> toolChains = toolChainManager.getToolChainsMatching(properties);
            if (toolChains.size() > 0) {
                myToolChain = toolChains.iterator().next();
                return myToolChain;
            } else {
                throw new CoreException(new Status(Status.ERROR, Activator.PLUGIN_ID,
                        "Failed to get toolchain for command " + command));
            }
        } else {
            myToolChain = null;
            return super.getToolChain();
        }
    }

    @Override
    public boolean processLine(String line, List<Job> jobsArray) {
        // Split line into args, taking into account quotes
        List<String> command = stripArgs(line);
        try {
            getToolChain(command);
        } catch (CoreException e1) {
            CCorePlugin.log(e1);
            return false;
        }
        return super.processLine(line, jobsArray);
    }

[jonahgraham]

FWIW this makes me double-down that change toolchain to non-final is a mistake. The code you have here leaves the toolchain in a non-deterministic state. So which toolchain getBinaryParserId uses will depend on what happened to be the last line processed in processLine.

---

OT but:

            Map<String, String> properties = new HashMap<String, String>() {
            private static final long serialVersionUID = 4493246628665145757L;

            {
                put(IToolChain.ATTR_ARCH, "myArch");
            }
        };

can probably be replaced with:

Map<String, String> properties = Map.of(IToolChain.ATTR_ARCH, "myArch");

[ewaterlander]

So, in your toolchain, can you do something like this?

I had to completely override getResourcesFromCommand(), because in my command line the C source file can be in the middle, followed by other arguments without a dash. The implementation of GCCToolChain assumes the C source file is the last item in the line. I'm checking the file extension.

I can use stripCommand() unmodified.

[ewaterlander]

Here with check on extension.

    @Override
    public IResource[] getResourcesFromCommand(List<String> cmd, URI buildDirectoryURI) {
        /*
         * Start at the back looking for arguments
         * GCCToolChain.getResourcesFromCommand() assumes the source files are at the
         * end of the command.
         */
        List<IResource> resources = new ArrayList<>();
        IWorkspaceRoot root = ResourcesPlugin.getWorkspace().getRoot();
        for (int i = cmd.size() - 1; i >= 0; --i) {
            String arg = cmd.get(i);
            if (arg.startsWith("-")) { //$NON-NLS-1$
                // ran into an option, we're done.
                continue;
            }
            if (i > 1 && cmd.get(i - 1).equals("-o")) { //$NON-NLS-1$
                // this is an output file
                --i;
                continue;
            }
            if (arg.endsWith(".c")) {
                try {
                    Path srcPath = Paths.get(arg);
                    URI uri;
                    if (srcPath.isAbsolute()) {
                        uri = srcPath.toUri();
                    } else {
                        String mingwPath = fixMingwPath(arg);
                        if (mingwPath != arg) {
                            uri = Paths.get(mingwPath).toUri();
                        } else {
                            uri = Paths.get(buildDirectoryURI).resolve(srcPath).toUri().normalize();
                        }
                    }

                    for (IFile resource : root.findFilesForLocationURI(uri)) {
                        resources.add(resource);
                    }
                } catch (IllegalArgumentException e) {
                    // Bad URI
                    continue;
                }
                // For now we assume one source file at a time.
                break;
            }
        }

        return resources.toArray(new IResource[resources.size()]);
    }

[ewaterlander]

MyToolChain.getResourcesFromCommand() is only called on lines which use myCompiler. So calling super is not needed.

[jonahgraham]

The implementation of GCCToolChain assumes the C source file is the last item in the line.

That is a defect that should be resolved in CDT. Would you be able to provide your code as a PR without the arg.endsWith(".c") part?

That said, I don't know why this code didn't reuse* o.e.c.m.l.s.providers.AbstractBuildCommandParser.parseResourceName(String) which handles all these cases and has many more years of testing under its belt (like o.e.c.m.l.s.providers.tests.GCCBuildCommandParserTest). My guess is that Doug was trying to simplify the code.

* The reuse could be code or knowledge. Parsing a gcc command line is complicated and the getResourcesFromCommand in GCCToolChain is way too over-simplified.

[ewaterlander]

That is a defect that should be resolved in CDT. Would you be able to provide your code as a PR without the arg.endsWith(".c") part?

That said, I don't know why this code didn't reuse* o.e.c.m.l.s.providers.AbstractBuildCommandParser.parseResourceName(String) which handles all these cases and has many more years of testing under its belt (like o.e.c.m.l.s.providers.tests.GCCBuildCommandParserTest). My guess is that Doug was trying to simplify the code.

• The reuse could be code or knowledge. Parsing a gcc command line is complicated and the getResourcesFromCommand in GCCToolChain is way too over-simplified.

OK. I will create a PR.

[github-actions]

Test Results

     602 files       602 suites   16m 47s ⏱️
10 183 tests 10 161 ✔️ 21 💤 1 ❌
10 146 runs  10 124 ✔️ 21 💤 1 ❌

For more details on these failures, see this check.

Results for commit d0d8ff6.

[jonahgraham]

Please disregard the failing test testClassInUnnamedNamespace (org.eclipse.cdt.internal.index.tests.IndexSearchTest) it is a flaky test and is covered in #117

[jonahgraham]

The CompositeToolChain looks like a good idea. As far as I see it can cover all my issues.

On this basis I am going to close this PR. I look forward to future discussions with you on this as you put this code through its paces.

[ewaterlander]

I look forward to future discussions with you on this as you put this code through its paces.

I will continue looking at this code. I'm not so well known with CDT code yet, and don't oversee the impact on the binary parser related code, but I'm learning. I also do have a practical problem. I'm working on Linux, but our environment is not the latest and greatest. At the moment I'm stuck at Eclipse 4.22/CDT 10.5. Latest Eclipse requires newer GTK+ libraries and Java 17. That doesn't run out-of-the-box in my environment.

[jonahgraham]

At the moment I'm stuck at Eclipse 4.22/CDT 10.5.

In that case I am glad we found a possible solution that works for you with CDT 10.5 without having to modify CDT. As you learn more I look forward to you sharing what you found and maybe one day we can move the core make feature out of experimental state.