ASF subversion and git services commented on MYNEWT-365:

Commit 5c5a987ef72ae9b3eb682f19d7d0bdc85f7e500c in incubator-mynewt-core's 
branch refs/heads/develop from [~ccollins476]
[ https://git-wip-us.apache.org/repos/asf?p=incubator-mynewt-core.git;h=5c5a987 

MYNEWT-365 newt can't handle files with spaces

This ticket's sister commit changed newt in a way that breaks
compatibility with the core repo.  This commit makes the core repo
compatible with newt again.

Newt no longer invokes /bin/sh to execute external commands.  Instead,
it now uses the Go exec library to fork+exec child processes.

NOTE: This commit breaks backwards-compatibility with the Mynewt core
repo.  Specifically, the compiler.yml definitions indicated command line
options as a space-delimited sequence of strings in the path settings.
E.g., the Cortex M4 assembler specified the following path:

    compiler.path.as: arm-none-eabi-gcc -x assembler-with-cpp

This compiler definition has now been changed to the following:

    compiler.path.as: arm-none-eabi-gcc
    compiler.as.flags: [-x, assembler-with-cpp]

We could have maintained backwards-compatibility by splitting paths on
spaces and interpreting secondary tokens as command-line options.
However, this approach prevents us from specifying a compiler path
that actually contains spaces.  I think paths with spaces is not
uncommon (e.g., "/Applications/GNU ARM Eclipse/...")

In addition, some packages' cflags contained shell-escaped strings
(e.g., backslash-quote).  These escapes had to be removed now that the
flags aren't passing through the shell.

> newt can't handle files with spaces
> -----------------------------------
>                 Key: MYNEWT-365
>                 URL: https://issues.apache.org/jira/browse/MYNEWT-365
>             Project: Mynewt
>          Issue Type: Bug
>          Components: Newt
>    Affects Versions: v0_9_0
>         Environment: Any
>            Reporter: Tim
>            Assignee: Christopher Collins
>            Priority: Minor
> I was just looking at the code that decides where gcc is run from (so I can 
> see if I can make it always run from /workspace, so full paths are used), 
> when I noticed you don't handle program arguments properly.
> See [this 
> code|https://github.com/apache/incubator-mynewt-newt/blob/8abc4f2c2bb1ab784a854cdf5662e79d88a41407/newt/toolchain/compiler.go#L262]
> {code}
>       cmd += " -c " + "-o " + objPath + " " + file +
>               " " + c.cflagsString() + " " + c.includesString()
> {code}
> Command line arguments should be stored as a {{[]string}} instead of a 
> space-delimited {{string}}. Then you don't need to worry about spaces or 
> escaping or anything like that. In other words something like this:
> {code}
> func (c *Compiler) CompileFileCmd(file string,
>       compilerType int) ([]string, error) {
>       objFile := strings.TrimSuffix(file, filepath.Ext(file)) + ".o"
>       objPath := filepath.ToSlash(c.dstDir + "/" + objFile)
>       cmd := make([]string)
>       switch compilerType {
>       case COMPILER_TYPE_C:
>               cmd = cmd.append(c.ccPath)
>       case COMPILER_TYPE_ASM:
>               cmd = cmd.append(c.asPath)
>       default:
>               return nil, util.NewNewtError("Unknown compiler type")
>       }
>       cmd = append(cmd, "-c", "-o", objPath, file)
>       // There will be some special handling for these, depending on what 
> they contain (I'm not sure of the format of these exactly).
> //    c.cflagsString(), c.includesString()
>       return cmd, nil
> }
> {code}
> And then don't use ShellCommand() to run it. Is there any reason that you're 
> using {{sh -c}} rather than just running the command directly? It's going to 
> make porting to Windows a pain. Similarly for code like this:
> {code}
> func CopyFile(srcFile string, destFile string) error {
>       _, err := ShellCommand(fmt.Sprintf("mkdir -p %s", 
> filepath.Dir(destFile)))
>       if err != nil {
>               return err
>       }
>       if _, err := ShellCommand(fmt.Sprintf("cp -Rf %s %s", srcFile,
>               destFile)); err != nil {
>               return err
>       }
>       return nil
> }
> {code}
> That won't work on Windows and also won't work with files containing spaces. 
> Better to use Go's proper functions for creating directories and copying 
> files. (Unfortunately there isn't a built-in equivalent of {{cp -Rf}} but if 
> you google it there is lots of example code.)

This message was sent by Atlassian JIRA

Reply via email to