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: Extract resources object and implement incremental scan #7183

Merged
merged 44 commits into from
Jul 17, 2018

Conversation

theotherjimmy
Copy link
Contributor

@theotherjimmy theotherjimmy commented Jun 11, 2018

Description

I found that the resource object being part of the tool chain, and depending on
tool chain infrastructure was a bit odd, so I extracted resources from tool
chains. The resulting code is quite a bit more clear in it's purpose and can be a
tad smaller (extra documentation and formatting excluded). I made the resources
object fully incremental, meaning that the scan stops early otfen for targets,
toolchains and features, and can easily pick itself back up where it left off.

A side effect of fully incremental scan is that configuration, which cannot depend
on a tool chain, does not need a tool chain specified to work correctly. I updated
get_config.py (mbed compile --config's back end) to reflect this.

Fixes #6038 (--source with a path starting with ../ shows incorrectly in memap)
Fixes #5357 (--source with uvision exporter creates broken project files)
(Possibly more as I find issues fixed by being up front about our data model)

TODO

  • replace that odd file_basepath member with something more suitable
  • finish replacing calls to toolchain.scan_resources and scan_resources
  • Document new architecture.

Pull request type

[ ] Fix
[x] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@theotherjimmy
Copy link
Contributor Author

@cmonr 🤞 I got the last "use paths" bug. That should unblock Oulu CI. Ready for review.

cmonr
cmonr previously requested changes Jun 18, 2018
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

LGTM. A couple of nits and questions, but should be good to go afterwards.

tools/.#make.py Outdated
@@ -0,0 +1 @@
jimmy@archlinux.3808:1528302063
Copy link
Contributor

Choose a reason for hiding this comment

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

Something tells me this shouldn't be 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.

Oh for sure. I'm going to rebase and purge it from the history.

# library

# A number of compiled files need to be copied as objects as opposed to
# way the linker search for symbols in archives. These are:
Copy link
Contributor

Choose a reason for hiding this comment

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

  • the way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol. I'll fix that.

@@ -976,141 +907,138 @@ def build_mbed_libs(target, toolchain_name, clean=False, macros=None,
properties - UUUUHHHHH beats me
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably get a better description at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At some point, when I see it used. (I have not seen it passed in)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just looked around, and it seems that there were plans for a thing called properties, but no implementation using them...

mbed_resources = Resources(notify).scan_with_toolchain(
[MBED_DRIVERS, MBED_PLATFORM, MBED_HAL], toolchain)

incdirs = cmsis_res.inc_dirs + hal_res.inc_dirs + library_incdirs
Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks.

@theotherjimmy theotherjimmy force-pushed the refactor-resources branch 2 times, most recently from b4a0ba2 to ba35a00 Compare June 19, 2018 14:57
@theotherjimmy
Copy link
Contributor Author

@cmonr I rebased out those problems that you commented on.

@theotherjimmy
Copy link
Contributor Author

Master:

$ wc ./BUILD/K64F/ARM/.includes_2e382264abc50dd02247baa7de4a1173.txt
    0   241 15017 ./BUILD/K64F/ARM/.includes_2e382264abc50dd02247baa7de4a1173.txt

This branch:

$ wc ./BUILD/K64F/ARM/.includes_05d475aa5eadf631d0b7eb0ab4459616.txt
    0   195 11711 ./BUILD/K64F/ARM/.includes_05d475aa5eadf631d0b7eb0ab4459616.txt

@theotherjimmy theotherjimmy force-pushed the refactor-resources branch 2 times, most recently from 2a0e535 to 0942442 Compare June 19, 2018 19:29
@theotherjimmy
Copy link
Contributor Author

mbed test not working. impl of copy_sources used to use file_basepath, now it needs an update.

@theotherjimmy
Copy link
Contributor Author

rebased.

@theotherjimmy
Copy link
Contributor Author

@cmonr Ready for review.

if options.tool is None:
args_error(parser, "argument -t/--toolchain is required")
toolchain = options.tool[0]
toolchain = options.tool[0] if options.tool is not None else None
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this couldn't be shrunk to options.tool[0] if options.tool else None?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. Originally we had to worry about empty strings and what not.

(headername, " ".join(locations)))
return count

def win_to_unix(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

cmonr
cmonr previously approved these changes Jun 26, 2018
@theotherjimmy
Copy link
Contributor Author

All conflicts were with my other PRs...

@cmonr
Copy link
Contributor

cmonr commented Jul 16, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 16, 2018

Build : SUCCESS

Build number : 2619
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/7183/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Jul 16, 2018

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2018

@cmonr
Copy link
Contributor

cmonr commented Jul 27, 2018

Looks like this PR has a dependency on another PR targeted for 5.10.

@theotherjimmy
Copy link
Contributor Author

As usual, dependency might be the wrong word here. It was definitely rebased on top of mayn other PRs.

@cmonr
Copy link
Contributor

cmonr commented Jul 28, 2018

Correct, it was. Not sure what a better term would be, but what I do know was that this PR made changes on top of this (#7133) PR, which caused pytest and build failures.

pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
Tools: Extract resources object and implement incremental scan
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade memap to show components of included libraries Uvision5: export from within a sub folder
5 participants