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

Resources: Avoid assuming that deps have a sane name #8696

Merged
merged 2 commits into from Nov 14, 2018

Conversation

Projects
None yet
5 participants
@theotherjimmy
Contributor

theotherjimmy commented Nov 9, 2018

Description

The prior fix assume that the dependencies through .lib references
would have a "sane" name. My definition of "sane" here is that the
reference will have a path that starts with the path to the .lib file
and removes the .lib suffix. The online compiler does not remove the
.lib suffix. Instead, it keeps it. This makes the string replacement
in the prior PR fail.

Also, this is faster, and simpler.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change
Resources: Avoid assuming that deps have a sane name
### Description

The prior fix assume that the dependencies through `.lib` references
would have a "sane" name. My definition of "sane" here is that the
reference will have a path that starts with the path to the `.lib` file
and _removes_ the `.lib` suffix. The online compiler does not remove the
`.lib` suffix. Instead, it keeps it. This makes the string replacement
in the prior PR fail.

Also, this is faster, and simpler.

### Pull request type

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

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:handle-parent-renames branch from d95e3d4 to 6e63aca Nov 9, 2018

@cmonr cmonr requested a review from ARMmbed/mbed-os-tools Nov 9, 2018

@@ -254,35 +254,15 @@ def add_file_ref(self, file_type, file_name, file_path):
ref = FileRef(file_name.replace(sep, self._sep), file_path)
else:
ref = FileRef(file_name, file_path)
self._file_refs[file_type].add(ref)
if file_type:
self._file_refs[file_type].add(ref)

This comment has been minimized.

@bridadan

bridadan Nov 12, 2018

Contributor

Is there any purpose of constructing the FileRef if file_type is falsey? If not, you might consider moving the file_type check to the top, so something like this (I also suggested a few other changes/refactors, feel free to ignore them if they don't make sense):

def add_file_ref(self, file_type, file_name, file_path):
    if not file_type:
        return
    if sep != self._sep:
        file_name = file_name.replace(sep, self._sep)
    self._file_refs[file_type].add(FileRef(file_name, file_path))

This comment has been minimized.

@theotherjimmy

theotherjimmy Nov 12, 2018

Contributor

There is not. I'll update it in a bit.

@bridadan

Looks ok!

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:handle-parent-renames branch from fc84d7b to 3db1a8a Nov 13, 2018

@cmonr cmonr added needs: CI and removed needs: work labels Nov 13, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Nov 13, 2018

Fixes #8716

@cmonr

This comment has been minimized.

Contributor

cmonr commented Nov 13, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Nov 13, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Nov 14, 2018

@0xc0170 0xc0170 merged commit 82f195c into ARMmbed:master Nov 14, 2018

18 checks passed

ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
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/unittests Success
Details
travis-ci/astyle Passed, 49 files (+0 files)
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9843 cycles (-395 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8372B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment