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.

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

tools: fix the path generated to the sct file #9966

Merged
merged 5 commits into from Apr 9, 2019

Conversation

Projects
None yet
6 participants
@naveenkaje
Copy link
Contributor

commented Mar 6, 2019

Description

The sct file path generated in the online compiler
is incorrect. Fix that by changing the correct_scatter_shebang
API to accept a FileRef object instead and use the path.

This change should go with change in online compiler that removes
the override for correct_scatter_shebang.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@naveenkaje naveenkaje requested review from bridadan and theotherjimmy and removed request for bridadan Mar 6, 2019

@naveenkaje naveenkaje force-pushed the naveenkaje:sct_fix branch Mar 6, 2019

tools/export/makefile/__init__.py Outdated
@@ -272,11 +272,10 @@ def generate(self):
if self.resources.linker_script:
sct_file = self.resources.get_file_refs(FileType.LD_SCRIPT)[-1]
new_script = self.toolchain.correct_scatter_shebang(
sct_file.path, join("..", dirname(sct_file.name)))
sct_file, dirname(sct_file.name))

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 6, 2019

Contributor

This indentation looks off. It looks like it's 12 spaces when it should be 4.

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Mar 7, 2019

Author Contributor

fixed

tools/export/uvision/__init__.py Outdated
sct_file_ref = self.toolchain.correct_scatter_shebang(sct_file_ref, dirname(sct_file_ref.name))
self.resources.add_file_ref(FileType.LD_SCRIPT, sct_file_ref.name, sct_file_ref.path)
ctx['linker_script'] = sct_file_ref.name
if ctx['linker_script'] != sct_file_ref.path:
self.generated_files.append(ctx['linker_script'])

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 6, 2019

Contributor

I'm pretty sure that this (and the if above it) is no longer needed, as the new scatter file is in the Resources object.

This comment has been minimized.

Copy link
@bridadan

bridadan Mar 6, 2019

Contributor

@theotherjimmy Does the old linker script need to be removed from Resources?

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Mar 7, 2019

Author Contributor

lines removed as per Jimmy's comment

@bridadan
Copy link
Contributor

left a comment

Really nice work! A few questions for @theotherjimmy as I'm still getting more familiar with this.

tools/toolchains/arm.py Outdated
@@ -284,8 +285,9 @@ def link(self, output, objects, libraries, lib_dirs, scatter_file):
if lib_dirs:
args.extend(["--userlibpath", ",".join(lib_dirs)])
if scatter_file:
new_scatter = self.correct_scatter_shebang(scatter_file)
args.extend(["--scatter", new_scatter])
scatter_name = split(scatter_file)[-1]

This comment has been minimized.

Copy link
@bridadan

bridadan Mar 6, 2019

Contributor

I don't think this is quite right, shouldn't this be the path to the scatter_file relative to the project root @theotherjimmy ?

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 7, 2019

Contributor

Yes, this should be:

Suggested change
scatter_name = split(scatter_file)[-1]
scatter_name = dirname(scatter_file)

This comment has been minimized.

Copy link
@bridadan

bridadan Mar 7, 2019

Contributor

I don't think dirname is correct either, right? That wouldn't include the actual file name.
Wouldn't it be closer to relpath?

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Mar 7, 2019

Author Contributor

suggested changes by Jimmy have been made

@naveenkaje naveenkaje force-pushed the naveenkaje:sct_fix branch Mar 7, 2019

@naveenkaje

This comment has been minimized.

Copy link
Contributor Author

commented Mar 7, 2019

Tested the latest patch-set 1f051ffc4400ef312d2bf434ce59e64d7f9b4c07 locally on top of mbed-os-5.11.5.

tools/toolchains/arm.py Outdated
from os.path import join, dirname, splitext, basename, exists, isfile
from os import makedirs, write, remove
from os.path import join, dirname, splitext, basename, exists, isfile, split
from os import makedirs, write, curdir, remove

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 7, 2019

Contributor

I don't think curdir is used. Could you remove this? or point me to where you use it?

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Mar 7, 2019

Author Contributor

curdir is removed

@cmonr cmonr added needs: work and removed needs: review labels Mar 7, 2019

@naveenkaje naveenkaje force-pushed the naveenkaje:sct_fix branch Mar 7, 2019

@theotherjimmy
Copy link
Contributor

left a comment

My mistake

tools/toolchains/arm.py Outdated
@@ -284,8 +285,9 @@ def link(self, output, objects, libraries, lib_dirs, scatter_file):
if lib_dirs:
args.extend(["--userlibpath", ",".join(lib_dirs)])
if scatter_file:
new_scatter = self.correct_scatter_shebang(scatter_file)
args.extend(["--scatter", new_scatter])
scatter_name = dirname(scatter_file)

This comment has been minimized.

Copy link
@theotherjimmy

theotherjimmy Mar 7, 2019

Contributor

Oh yeah, my recommendation was really wrong here. This should probably be:

Suggested change
scatter_name = dirname(scatter_file)
scatter_name = relpath(scatter_file)

This comment has been minimized.

Copy link
@naveenkaje

naveenkaje Mar 7, 2019

Author Contributor

Changes made.

@naveenkaje naveenkaje force-pushed the naveenkaje:sct_fix branch to 8e59caa Mar 7, 2019

Show resolved Hide resolved tools/export/uvision/__init__.py Outdated
@theotherjimmy

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

For what it's worth: I have tested this locally for compatibility with the online compiler, and it works

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 25, 2019

@naveenkaje Fyi, this needs a rebase.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

@naveenkaje Fyi, this needs a rebase.

Ping

tools: fix the path generated to the sct file
The sct file path generated in the online compiler
is incorrect. Fix that by changing the correct_scatter_shebang
API to accept a FileRef object instead and use the path.

This change should go with change in online compiler that removes
the override for correct_scatter_shebang.

@bridadan bridadan force-pushed the naveenkaje:sct_fix branch from 8e59caa to f0f133f Apr 2, 2019

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

Rebased!

@cmonr cmonr added needs: CI and removed needs: work labels Apr 2, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

CI started

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 3, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Apr 3, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

make armc5/6 failures in exporters - please review

@bridadan bridadan requested a review from theotherjimmy Apr 5, 2019

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

It'd be worth having @theotherjimmy take another look due to the new changes.

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

Oh I didn't mention this explicitly, but the exporter failures should be addressed with these latest changes.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 9, 2019

Test run: FAILED

Summary: 1 of 13 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_exporter

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Apr 9, 2019

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

Ok, last issue should be resolved.

@0xc0170 0xc0170 added needs: CI and removed needs: work labels Apr 9, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

commented Apr 9, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

@cmonr cmonr added ready for merge and removed needs: CI labels Apr 9, 2019

@cmonr cmonr merged commit 803f5fd into ARMmbed:master Apr 9, 2019

26 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9964 cycles (+800 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@cmonr cmonr removed the ready for merge label Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.