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.

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

Put quotes around include files #4468

Merged
merged 1 commit into from Jul 6, 2017

Conversation

Projects
None yet
7 participants
@moonchen
Contributor

moonchen commented Jun 6, 2017

This fixes a problem when the path to include files have spaces.
See ARMmbed/mbed-os-example-uvisor#31 for an
example of this problem.

Signed-off-by: Mo Chen mo.chen@arm.com

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 6, 2017

@moonchen Have you confirmed that this input is accepted by all three toolchains?

@sg- sg- added the needs: review label Jun 7, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 9, 2017

@moonchen bump

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 13, 2017

Let's run CI on this to confirm that this would not fail for any toolchain, as this should be exercised there.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Jun 13, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 13, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 14, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 548

Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 14, 2017

@moonchen IAR failed for all targets, looks like this is not supported there.

Locally, it fails for any assembly file as:

Compile [100.0%]: cmain.S
Traceback (most recent call last):
  File "tools\build.py", line 211, in <module>
    build_profile=profile)
  File "mbed-os\tools\build_api.py", line 1014, in build_mbed_libs
    objects = toolchain.compile_sources(resources, tmp_path)
  File "mbed-os\tools\toolchains\__init__.py", line 830, in compile_sources
    return self.compile_seq(queue, objects)
  File "mbed-os\tools\toolchains\__init__.py", line 844, in compile_seq
    res['command']
  File "mbed-os\tools\toolchains\__init__.py", line 986, in compile_output
    raise ToolException(_stderr)
ToolException

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Jun 14, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 14, 2017

Instead of wrapping in quotes, what about escaping the spaces?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 14, 2017

@bridadan @moonchen How about wrapping the whole argument in quotes?

@bridadan

This comment has been minimized.

Contributor

bridadan commented Jun 14, 2017

Worth a shot. @moonchen do you have access to the IAR compiler?

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 14, 2017

@moonchen I gave it a quick whirl.

diff --git i/tools/toolchains/__init__.py w/tools/toolchains/__init__.py
index c572fe96a..8aef56b22 100644
--- i/tools/toolchains/__init__.py
+++ w/tools/toolchains/__init__.py
@@ -740,7 +740,7 @@ class mbedToolchain:
                         c = c.replace("\\", "/")
                         if self.CHROOT:
                             c = c.replace(self.CHROOT, '')
-                        cmd_list.append('-I%s' % c)
+                        cmd_list.append('"-I%s"' % c)
                 string = " ".join(cmd_list)
                 f.write(string)
         return include_file

does not error in that way

@moonchen

This comment has been minimized.

Contributor

moonchen commented Jun 16, 2017

@theotherjimmy quoting the whole thing worked for me on gcc.

@theotherjimmy theotherjimmy force-pushed the moonchen:quote-include-paths branch Jun 16, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 16, 2017

Then let's use that commit.

@theotherjimmy theotherjimmy force-pushed the moonchen:quote-include-paths branch Jun 16, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 16, 2017

@moonchen @bridadan I already tested this with ARMCC and IAR.

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Jun 19, 2017

Put quotes around include files
This fixes a problem when the path to include files have spaces.
See ARMmbed/mbed-os-example-uvisor#31 for an
example of this problem.

Signed-off-by: Mo Chen <mo.chen@arm.com>

@moonchen moonchen force-pushed the moonchen:quote-include-paths branch to 1b63202 Jun 20, 2017

@moonchen

This comment has been minimized.

Contributor

moonchen commented Jun 20, 2017

Amended to use the new patch.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 20, 2017

@moonchen I already did that.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 26, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 27, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 644

Test failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 29, 2017

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 691

Test failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Jun 29, 2017

These timer tests need to be way less flaky.

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Jun 30, 2017

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 703

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 30, 2017

retest uvisor

@theotherjimmy theotherjimmy merged commit 99b37b2 into ARMmbed:master Jul 6, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 12, 2017

Hi
Since this patch, I couldn't execute IAR tests...

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 12, 2017

$ python tools/build.py -m NUCLEO_F411RE -t IAR -vv
Building library CMSIS (NUCLEO_F411RE, IAR)
Scan: cmsis
[DEBUG] Macros: -D__MBED__=1 -DDEVICE_I2CSLAVE=1 -D__FPU_PRESENT=1 -DDEVICE_PORTOUT=1 -DUSBHOST_OTHER -DDEVICE_PORTINOUT=1 -DTARGET_RTOS_M4_M7 -DDEVICE_LOWPOWERTIMER=1 -DDEVICE_RTC=1 -DTOOLCHAIN_object -DDEVICE_SERIAL_ASYNCH=1 -DTARGET_STM32F4 -D__CMSIS_RTOS -D__CORTEX_M4 -DDEVICE_I2C_ASYNCH=1 -DTARGET_CORTEX_M -DTARGET_LIKE_CORTEX_M4 -DTARGET_M4 -DTARGET_UVISOR_UNSUPPORTED -DDEVICE_SPI_ASYNCH=1 -DTARGET_STM32F411xE -DTOOLCHAIN_IAR -DDEVICE_INTERRUPTIN=1 -DMBED_BUILD_TIMESTAMP=1499852438.58 -DDEVICE_I2C=1 -DTRANSACTION_QUEUE_SIZE_SPI=2 -DTARGET_NUCLEO_F411RE -DDEVICE_STDIO_MESSAGES=1 -DDEVICE_SERIAL=1 -DTARGET_FF_MORPHO -DTARGET_FAMILY_STM32 -DTARGET_FF_ARDUINO -DDEVICE_PORTIN=1 -DTARGET_RELEASE -DTARGET_STM -DDEVICE_SERIAL_FC=1 -DTARGET_LIKE_MBED -D__MBED_CMSIS_RTOS_CM -DDEVICE_SLEEP=1 -DDEVICE_SPI=1 -DUSB_STM_HAL -DDEVICE_ERROR_RED=1 -DDEVICE_SPISLAVE=1 -DDEVICE_ANALOGIN=1 -DDEVICE_PWMOUT=1 -DTARGET_STM32F411RE -DARM_MATH_CM4
Compile [100.0%]: cmain.S
[DEBUG] Compile: C:\Program Files (x86)\IAR Systems\Embedded Workbench 7.5\arm\bin\iasmarm --cpu Cortex-M4F -DTRANSACTION_QUEUE_SIZE_SPI=2 -D__CORTEX_M4 -DUSB_STM_HAL -DARM_MATH_CM4 -D__FPU_PRESENT=1 -DUSBHOST_OTHER -D__MBED_CMSIS_RTOS_CM -D__CMSIS_RTOS -f C:\github\mbed\BUILD\mbed.temp\TARGET_NUCLEO_F411RE\TOOLCHAIN_IAR.includes_4cfbfefad6235a1de6e720e267b71586.txt -o C:\github\mbed\BUILD\mbed.temp\TARGET_NUCLEO_F411RE\TOOLCHAIN_IAR\TOOLCHAIN_IAR\cmain.o C:\github\mbed\cmsis\TOOLCHAIN_IAR\cmain.S
[DEBUG] Return: 3
Traceback (most recent call last):
File "tools/build.py", line 212, in
build_profile=profile)
File "C:\github\mbed\tools\build_api.py", line 1014, in build_mbed_libs
objects = toolchain.compile_sources(resources, tmp_path)
File "C:\github\mbed\tools\toolchains_init_.py", line 908, in compile_sources
return self.compile_seq(queue, objects)
File "C:\github\mbed\tools\toolchains_init_.py", line 922, in compile_seq
res['command']
File "C:\github\mbed\tools\toolchains_init_.py", line 1064, in compile_output
raise ToolException(_stderr)
ToolException

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2017

@jeromecoutant Thanks for reporting, I'll check what is going on, I am able to reproduce it, and will cehck what CI did for this PR.

It is not this PR, If you revert it, you get the same ToolExpection, investigating

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jul 12, 2017

When I revert it, it works for me

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 12, 2017

When I revert it, it works for me

I sent a fix, please review it #4749

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