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

naveenkaje · 2019-03-06T20:36:38Z

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

theotherjimmy · 2019-03-06T20:37:46Z

tools/export/makefile/__init__.py

@@ -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 indentation looks off. It looks like it's 12 spaces when it should be 4.

theotherjimmy · 2019-03-06T20:38:34Z

tools/export/uvision/__init__.py

+        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'])


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.

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

lines removed as per Jimmy's comment

bridadan

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

bridadan · 2019-03-06T21:17:23Z

tools/toolchains/arm.py

@@ -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]


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

Yes, this should be:

Suggested change

scatter_name = split(scatter_file)[-1]

scatter_name = dirname(scatter_file)

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

suggested changes by Jimmy have been made

naveenkaje · 2019-03-07T12:28:26Z

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

theotherjimmy · 2019-03-07T16:41:21Z

tools/toolchains/arm.py

-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


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

curdir is removed

theotherjimmy

My mistake

theotherjimmy · 2019-03-07T19:44:35Z

tools/toolchains/arm.py

@@ -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)


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

Suggested change

scatter_name = dirname(scatter_file)

scatter_name = relpath(scatter_file)

Changes made.

tools/export/uvision/__init__.py

theotherjimmy · 2019-03-08T19:31:23Z

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

cmonr · 2019-03-25T18:40:23Z

@naveenkaje Fyi, this needs a rebase.

cmonr · 2019-04-01T21:12:17Z

@naveenkaje Fyi, this needs a rebase.

Ping

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 · 2019-04-02T14:08:03Z

Rebased!

cmonr · 2019-04-02T14:22:31Z

CI started

0xc0170 · 2019-04-03T07:11:34Z

CI restarted

mbed-ci · 2019-04-03T08:54:56Z

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 · 2019-04-03T09:09:57Z

make armc5/6 failures in exporters - please review

bridadan · 2019-04-05T16:53:27Z

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

bridadan · 2019-04-08T20:20:18Z

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

cmonr · 2019-04-08T21:08:36Z

CI started

mbed-ci · 2019-04-09T03:50:00Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_exporter

bridadan · 2019-04-09T15:22:09Z

Ok, last issue should be resolved.

cmonr · 2019-04-09T16:08:52Z

CI started

mbed-ci · 2019-04-09T18:14:25Z

Test run: SUCCESS

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

naveenkaje requested review from bridadan and theotherjimmy and removed request for bridadan March 6, 2019 20:36

naveenkaje force-pushed the sct_fix branch from 1dd23b9 to 5babd54 Compare March 6, 2019 20:42

theotherjimmy suggested changes Mar 6, 2019

View reviewed changes

bridadan reviewed Mar 6, 2019

View reviewed changes

naveenkaje force-pushed the sct_fix branch from 5babd54 to 1f051ff Compare March 7, 2019 12:26

cmonr added needs: review release-version: 5.12.1 labels Mar 7, 2019

theotherjimmy suggested changes Mar 7, 2019

View reviewed changes

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

naveenkaje force-pushed the sct_fix branch from 1f051ff to 8bc1e02 Compare March 7, 2019 19:31

theotherjimmy suggested changes Mar 7, 2019

View reviewed changes

naveenkaje force-pushed the sct_fix branch from 8bc1e02 to 8e59caa Compare March 7, 2019 22:00

bridadan suggested changes Mar 8, 2019

View reviewed changes

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

theotherjimmy approved these changes Mar 8, 2019

View reviewed changes

bridadan approved these changes Mar 8, 2019

View reviewed changes

bridadan force-pushed the sct_fix branch from 8e59caa to f0f133f Compare April 2, 2019 14:06

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

Test to ensure only the last linker file is used in a project

fd7467c

0xc0170 added the needs: CI label Apr 3, 2019

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

Correct call to add_file_ref

2a080a0

bridadan requested a review from theotherjimmy April 5, 2019 16:53

theotherjimmy approved these changes Apr 5, 2019

View reviewed changes

bridadan mentioned this pull request Apr 8, 2019

Enable post build bootloader merging in uvision #10021

Merged

cmonr added needs: CI release-version: 5.12.2 and removed needs: work release-version: 5.12.1 labels Apr 8, 2019

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

Make files call the linker from the build dir, so correct the path

80b0a4c

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

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

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

cmonr removed the ready for merge label Apr 9, 2019

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

	scatter_name = dirname(scatter_file)
	scatter_name = relpath(scatter_file)

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

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

Uh oh!

Conversation

naveenkaje commented Mar 6, 2019

Description

Pull request type

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridadan Mar 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridadan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveenkaje commented Mar 7, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theotherjimmy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

theotherjimmy commented Mar 8, 2019

Uh oh!

cmonr commented Mar 25, 2019

Uh oh!

cmonr commented Apr 1, 2019

Uh oh!

bridadan commented Apr 2, 2019

Uh oh!

cmonr commented Apr 2, 2019

Uh oh!

0xc0170 commented Apr 3, 2019

Uh oh!

mbed-ci commented Apr 3, 2019

Uh oh!

0xc0170 commented Apr 3, 2019

Uh oh!

bridadan commented Apr 5, 2019

Uh oh!

bridadan commented Apr 8, 2019

Uh oh!

cmonr commented Apr 8, 2019

Uh oh!

mbed-ci commented Apr 9, 2019

Uh oh!

bridadan commented Apr 9, 2019

Uh oh!

cmonr commented Apr 9, 2019

Uh oh!

mbed-ci commented Apr 9, 2019

Uh oh!

Uh oh!

bridadan Mar 6, 2019 •

edited

Loading