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

Don't set the build_dir to anything on export #3924

Merged
merged 1 commit into from Mar 22, 2017

Conversation

Projects
None yet
6 participants
@theotherjimmy
Contributor

theotherjimmy commented Mar 10, 2017

When constructing a toolchain for export, we currently set the
build_dir to the export_dir. When exporting offline, the
export_dir is always set to the root of the project. The toolchains
ignore their build_dir when scanning for sorces, so when the exporters
use the toolchains to scan for their resources, they get nothing.

In this patch we set the build_dir of the toolchain that the exports
use to nothing. A path of nothing should not match anything, and will
therefore not ignore everything when scanning for resources.

TODO

  • /morph export-build
@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 10, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 10, 2017

Result: SUCCESS

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

/morph export-build

Output

mbed Build Number: 135

All exports and builds passed!

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:fix-export-build-dir branch Mar 10, 2017

@sg-

This comment has been minimized.

Member

sg- commented Mar 11, 2017

/morph export-build

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 11, 2017

Result: SUCCESS

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

/morph export-build

Output

mbed Build Number: 136

All exports and builds passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 13, 2017

I restarted jenkins CI as I could not locate the test results, please @theotherjimmy review once it finishes

@0xc0170

LGTM

Small typo in the commit message- bulid (build?). I had to look at the prepare_toolchian definition to see what is being set to "". It was not clear to me from the commit.

@0xc0170 0xc0170 added the needs: CI label Mar 13, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 14, 2017

Restarted CI jenkins to report back the status

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 14, 2017

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 14, 2017

@theotherjimmy "We are not using the toolchains to bulid, and they ignore everything in
the build_dir when scanning, so let's not ignore everything" Could you please re-write this to a meaningful comment that everybody can understand.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 14, 2017

Ah, I missed the the tool chains have the build dir set to the root of the project bit

Don't set the build_dir to anything on export
When constructing a toolchain for export, we currently set the
`build_dir` to the `export_dir`. When exporting offline, the
`export_dir` is always set to the root of the project. The toolchains
ignore their `build_dir` when scanning for sorces, so when the exporters
use the toolchains to scan for their resources, they get nothing.

In this patch we set the `build_dir` of the toolchain that the exports
use to nothing. A path of nothing should not match anything, and will
therefore not ignore everything when scanning for resources.

@theotherjimmy theotherjimmy force-pushed the theotherjimmy:fix-export-build-dir branch to 4219d9c Mar 14, 2017

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 14, 2017

Commit message changed. Copying to PR description.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 14, 2017

/morph export-build

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 14, 2017

'cause rebase

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 14, 2017

@adbridge I changed the commit message to be more useful (and updated the PR description to match it). Could you comment again?

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 14, 2017

@theotherjimmy Thanks for the updated description, much improved.

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Mar 14, 2017

@adbridge Your welcome. I'm changing it to needs: CI then.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 14, 2017

@marhil01 @SeppoTakalo Any idea why pr_head is failing here? I couldn't see an obvious failure?

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 14, 2017

Result: SUCCESS

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

/morph export-build

Output

mbed Build Number: 138

All exports and builds passed!

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 14, 2017

Looks like there's a build failure with K64F for the mbed-client-testapp?

18:27:49 [K64F ARM] "/tmp/4Bnhfm", line 128 (column 3): Warning: L6312W: Empty Execution region description for region RW_IRAM1
18:27:49 [K64F ARM] Error: L6200E: Symbol __asm___12_Ethernet_cpp___Z7__REV16j multiply defined (by Ethernet.o and CAN.o).
18:27:49 [K64F ARM] Error: L6200E: Symbol __asm___12_Ethernet_cpp___Z7__REVSHi multiply defined (by Ethernet.o and CAN.o).
18:27:49 [K64F ARM] Error: L6200E: Symbol __asm___12_Ethernet_cpp___Z5__RRXj multiply defined (by Ethernet.o and CAN.o).
18:27:49 [K64F ARM] Not enough information to list the image map.
18:27:49 [K64F ARM] Finished: 1 information, 1 warning and 3 error messages.
18:27:49 [K64F ARM] [ERROR] "/tmp/4Bnhfm", line 128 (column 3): Warning: L6312W: Empty Execution region description for region RW_IRAM1
18:27:49 [K64F ARM] Error: L6200E: Symbol __asm___12_Ethernet_cpp___Z7__REV16j multiply defined (by Ethernet.o and CAN.o).
18:27:49 [K64F ARM] Error: L6200E: Symbol __asm___12_Ethernet_cpp___Z7__REVSHi multiply defined (by Ethernet.o and CAN.o).
18:27:49 [K64F ARM] Error: L6200E: Symbol __asm___12_Ethernet_cpp___Z5__RRXj multiply defined (by Ethernet.o and CAN.o).
@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 15, 2017

@bridadan where is the failure? All tests are now green?

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 15, 2017

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 15, 2017

@0xc0170 I had restarted the CI once more after posting that, glad the issue is resolved!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 16, 2017

@theotherjimmy This is a bugfix for a bug that is on master and even on 5.4 branch (I faced this issue locally and took me a while to figure out that this is not my local setup but upstream bug). The importance of this bug is higher than it seems. It clicked all once I started browsing and saw all the reports (were referenced only 2 days ago, but this patch was sent 6 days ago thus we could already know more about this bug 6 days ago?). This could be easily integrated for the last release but was not as the description was lacking details, and even after improved, I still don't find it enough. For instance, when was this bug introduced? I had to go to the history, this one : theotherjimmy@fbb6f71. So this is fixing already another fix that introduced this bug. How can I reproduce this bug ? what does this bugfix fixes? As in the commit message : Before this patch: description here. After this patch: description here.

Please take more time to describe the issue, how this patch fixes it and reference issues or any relevant information. I assume this bugfix could be already in the last release!

How come this was not discovered with our build test that the project even fails to export or exported project is empty (only root is scanned, and in my case it was just config header file in the workspace view) ? @theotherjimmy Did you check build tests? I checked the PR that introduced this change, we did not run build CI test ! Here's it : #3852 (no mention of running build CI). Would it fail?

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 16, 2017

To help prevent this from happening in the future, I have a test job that will replace the nightly. It will test the exporters in parallel to running tests on the hardware, so you shouldn't have to run the explicit exporter job any more.

@adbridge

This comment has been minimized.

Contributor

adbridge commented Mar 20, 2017

@bridadan Do you mean that when we run /morph test-nightly going forward it will automatically test exporters as well? Or do you mean you have added a new, separate command ? If it's the latter +1 if it's the former +5 !

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 20, 2017

Result: FAILURE

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

/morph test-nightly going forward it will automatically test exporters as well? Or do you mean you have added a new, separate command ? If it's the latter +1 if it's the former +5 !

Output

mbed Build Number: 1700

Example Build failed!

@bridadan

This comment has been minimized.

Contributor

bridadan commented Mar 20, 2017

@adbridge It's not active now, but going forward the plan is to have the nightly command run the exporters as well. There a still a few issues to iron out. But this really needs to get in first!

@sg-

This comment has been minimized.

Member

sg- commented Mar 20, 2017

/morph test-nightly

@mbed-bot

This comment has been minimized.

mbed-bot commented Mar 21, 2017

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1706

All builds and test passed!

@sg- sg- merged commit 16304ae into ARMmbed:master Mar 22, 2017

5 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-export-build Job has started
Details
ci/morph-test-nightly 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment