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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions core/org.eclipse.cdt.core/src/org/eclipse/cdt/core/build/CBuildConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ public abstract class CBuildConfiguration extends PlatformObject implements ICBu

private final String name;
private final IBuildConfiguration config;
private final IToolChain toolChain;
private IToolChain toolChain;
private String launchMode;

private Object scannerInfoLock = new Object();
Expand Down Expand Up @@ -309,6 +309,17 @@ public IToolChain getToolChain() throws CoreException {
return toolChain;
}

/**
* setToolChain() can be used by sub-classes that need to support multiple C compilers
* in one Makefile. The sub-classes can override the processLine() methods, and
* set a different toolChain based on the compiler used.
*
* @param toolChain
*/
protected void setToolChain(IToolChain toolChain) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@ewaterlander ewaterlander Nov 1, 2022 •
edited by jonahgraham
Loading

Choose a reason for hiding this comment

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

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);
    }

Copy link
Member

Choose a reason for hiding this comment

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

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");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@ewaterlander ewaterlander Nov 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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()]);
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

this.toolChain = toolChain;
}

@Override
public IEnvironmentVariable getVariable(String name) {
IEnvironmentVariable[] vars = getVariables();
Expand Down Expand Up @@ -747,23 +758,12 @@ private void processElementDelta(ICElementDelta delta) {
}
}

/**
* Parse a string containing compile options into individual argument strings.
*
* @param argString - String to parse
* @return List of arg Strings
*/
private List<String> stripArgs(String argString) {
String[] args = CommandLineUtil.argumentsToArray(argString);
return new ArrayList<>(Arrays.asList(args));
}

private boolean infoChanged = false;

@Override
public boolean processLine(String line) {
// Split line into args, taking into account quotes
List<String> command = stripArgs(line);
List<String> command = CommandLineUtil.argumentsToList(line);

// Make sure it's a compile command
String[] compileCommands = toolChain.getCompileCommands();
Expand Down Expand Up @@ -891,7 +891,7 @@ protected IStatus run(IProgressMonitor monitor) {
@Override
public boolean processLine(String line, List<Job> jobsArray) {
// Split line into args, taking into account quotes
List<String> command = stripArgs(line);
List<String> command = CommandLineUtil.argumentsToList(line);

// Make sure it's a compile command
String[] compileCommands = toolChain.getCompileCommands();
Expand Down
13 changes: 13 additions & 0 deletions core/org.eclipse.cdt.core/utils/org/eclipse/cdt/utils/CommandLineUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
package org.eclipse.cdt.utils;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;

import org.eclipse.core.runtime.Platform;
import org.eclipse.osgi.service.environment.Constants;
Expand Down Expand Up @@ -46,6 +48,17 @@ public static String[] argumentsToArray(String line) {
}
}

/**
* Parse a string containing compile options into individual argument strings.
*
* @param line - String to parse
* @return List of arg Strings
*/
public static List<String> argumentsToList(String line) {
String[] args = CommandLineUtil.argumentsToArray(line);
return new ArrayList<>(Arrays.asList(args));
}

/**
* Parsing arguments in a shell style. i.e.
*
Expand Down