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

Add shareable instances of config.toml and openocd.gdb #308

Merged

Conversation

winksaville
Copy link
Contributor

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

ry.x:5: region 'FLASH' already defined
>>> FLASH : ORIGIN = 0x08000000, LENGTH = 256K
>>>

@winksaville winksaville requested a review from a team as a code owner March 5, 2021 00:14
@rust-highfive
Copy link

r? @adamgreig

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources labels Mar 5, 2021
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

src/06-hello-world/.cargo/config Show resolved Hide resolved
eldruin
eldruin previously approved these changes Mar 5, 2021
Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you!

bors r+

bors bot added a commit that referenced this pull request Mar 5, 2021
308: Add shareable instances of config.toml and openocd.gdb r=eldruin a=winksaville

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>

Co-authored-by: Wink Saville <wink@saville.com>
@bors
Copy link
Contributor

bors bot commented Mar 5, 2021

Build failed:

@eldruin
Copy link
Member

eldruin commented Mar 5, 2021

bors retry

bors bot added a commit that referenced this pull request Mar 5, 2021
308: Add shareable instances of config.toml and openocd.gdb r=eldruin a=winksaville

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>

Co-authored-by: Wink Saville <wink@saville.com>
@bors
Copy link
Contributor

bors bot commented Mar 5, 2021

Build failed:

@eldruin
Copy link
Member

eldruin commented Mar 5, 2021

bors retry

bors bot added a commit that referenced this pull request Mar 5, 2021
308: Add shareable instances of config.toml and openocd.gdb r=eldruin a=winksaville

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>

Co-authored-by: Wink Saville <wink@saville.com>
@bors
Copy link
Contributor

bors bot commented Mar 5, 2021

Build failed:

@eldruin
Copy link
Member

eldruin commented Mar 5, 2021

It seems building the book just takes too long on Travis :/

bors retry

bors bot added a commit that referenced this pull request Mar 5, 2021
308: Add shareable instances of config.toml and openocd.gdb r=eldruin a=winksaville

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>

Co-authored-by: Wink Saville <wink@saville.com>
@bors
Copy link
Contributor

bors bot commented Mar 5, 2021

Build failed:

@adamgreig
Copy link
Member

I wonder if something is actually wrong - does it work building locally?

@adamgreig
Copy link
Member

Oh, nevermind, the CI passed on the commit itself, so it should be fine. Just Travis things then?

@winksaville
Copy link
Contributor Author

Do you want me to do anything?

@adamgreig
Copy link
Member

Let's give it one more go on Travis...

bors retry

bors bot added a commit that referenced this pull request Mar 6, 2021
308: Add shareable instances of config.toml and openocd.gdb r=eldruin a=winksaville

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>

Co-authored-by: Wink Saville <wink@saville.com>
@bors
Copy link
Contributor

bors bot commented Mar 6, 2021

Build failed:

@winksaville
Copy link
Contributor Author

Is this failing during ci/script.sh that worked when building on the branch?

@winksaville
Copy link
Contributor Author

Is this failing during ci/script.sh that worked when building on the branch?

So I've done some investigation and there is a difference between how CI works for testing the PR itself and when it's merged. When "testing the PR" mdbook build is run in from ci/after-success while it's run from ci/script.sh when merging because while merging the branch is staging.

The problem appears that mdbook just takes a long time. I modified ci/scripts.sh to adding time RUST_LOG ...:

$ git diff master -- ci/script.sh
diff --git a/ci/script.sh b/ci/script.sh
index acfe2d2..ab6f593 100644
--- a/ci/script.sh
+++ b/ci/script.sh
@@ -2,10 +2,10 @@ set -euxo pipefail
 
 main() {
     # test that building the book works
-    mdbook build
+    time RUST_LOG=error,warn,info mdbook build
 
     # mdbook doesn't handle relative links correctly in print.html so skip it.
-    linkchecker --ignore-url "print.html" book
+    time linkchecker --ignore-url "print.html" book
 
     # now check this as a directory of the bookshelf
     rm -rf shelf

And it took 7 minutes to run mdbook build on Travis when testing the trying branch on my fork.

572.35s$ bash ci/script.sh
+'[' trying '!=' master ']'
+main
+RUST_LOG=error,warn,info
+mdbook build
2021-03-06 20:27:48 [INFO] (mdbook::book): Book building has started
2021-03-06 20:27:48 [INFO] (mdbook::book): Running the html backend
real	7m7.094s
user	0m2.273s
sys	0m10.127s

Although for some reason on my computer it takes 5 seconds, which is weird!
I do have a reasonably quick computer, AMD 3900X with SSD's but it is still
surprising.

$ time TRAVIS_BRANCH=trying bash ci/script.sh
+ '[' trying '!=' master ']'
+ main
+ RUST_LOG=error,warn,info
+ mdbook build
2021-03-06 12:55:00 [INFO] (mdbook::book): Book building has started
2021-03-06 12:55:00 [INFO] (mdbook::book): Running the html backend

real	0m5.397s
user	0m0.312s
sys	0m5.047s
+ linkchecker --ignore-url print.html book
Hello, world!

real	0m0.001s
user	0m0.001s
sys	0m0.000s

Anyway, one solution I thought of was to see I could get mdbook build to output
additional logging information. So I discovered it supported RUST_LOG and as
you can see I have it logging error,warn,info. I chose those because when it was
RUST_LOG=error,warn,info,debug, here, I got an error job exceeded the maximum log length:

2021-03-06 20:24:09 [DEBUG] (mdbook::utils::fs): creating path for file: "/home/travis/build/winksaville/rust-embedded-discovery/book/09-clocks-and-timers/target/thumbv7em-none-eabihf/debug/incremental/aux11-31llwmlgkbkg3/s-fwgnctax4m-mog78m-qtq3c2kgmfbo/query-cache.bin"
2021-03-06 20:24:09 [DEBUG] (mdbook::utils::fs): Copying "/home/travis/build/winksaville/rust-embedded-discovery/src/09-clocks-and-timers/target/thumbv7em-none-eabihf/debug/incremental/aux11-31llwmlgkbkg3/s-fwgnctax4m-mog78m-qtq3c2kgmfbo/query-cache.bin" to "/home/travis/build/winksaville/rust-embedded-discovery/book/09-clocks-and-timers/target/thumbv7em-none-eabihf/debug/incremental/aux11-31llwmlgkbkg3/s-fwgnctax4m-mog78m-qtq3c2kgmfbo/query-cache.bin"
The job exceeded the maximum log length, and has been terminated.

So there appears to be at least three solutions:

  1. Figure out why mdbook build is so slow

  2. Modify mdbook so that RUST_LOG=info will output a little more information,
    like displaying the name of each .md file it's processing.

  3. Use a different CI, maybe github actions.

Other idea's?

@eldruin
Copy link
Member

eldruin commented Mar 8, 2021

Thank you for your investigation. We have switched many projects already to GitHub Actions but this one is more complicated. Travis performance has pretty much made an U-turn. See rust-embedded/book#285.

bors bot added a commit that referenced this pull request Mar 8, 2021
310: Increase Travis no-output waiting time r=therealprof a=eldruin

Hopefully this will allow us to merge #308 

Co-authored-by: Diego Barrios Romero <eldruin@gmail.com>
@eldruin
Copy link
Member

eldruin commented Mar 8, 2021

@winksaville could you rebase this onto master? I increased the allowed waiting time in #310

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>
@winksaville
Copy link
Contributor Author

@winksaville could you rebase this onto master? I increased the allowed waiting time in #310

Done

Copy link
Member

@eldruin eldruin left a comment

Choose a reason for hiding this comment

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

Thank you! Let's see if we get it through this time.

bors r+

bors bot added a commit that referenced this pull request Mar 8, 2021
308: Add shareable instances of config.toml and openocd.gdb r=eldruin a=winksaville

Instead of every chapter having its own copy of the cargo configuration
and openocd.gdb files this change creates a shared set. This will make it
much easier for the user of the discovery book to handle the situation
where their Arm gdb is NOT arm-none-eabi-gdb. As only the shared copy
of .cargo/config.toml will have to be modified.

Also src/05-led-roulette is updated to take advantage of this and I
will go through each of the other chapters changing them to use
the shared set.

I also needed to comment out the rustflags variable in all of the config
files as I'd get an error executing ci/script.sh:

  ry.x:5: region 'FLASH' already defined
          >>>   FLASH : ORIGIN = 0x08000000, LENGTH = 256K
          >>>

Co-authored-by: Wink Saville <wink@saville.com>
@bors
Copy link
Contributor

bors bot commented Mar 8, 2021

Timed out.

@winksaville
Copy link
Contributor Author

Bummer, looks like it got stuck in the job queue and hasn't run, although I could be wrong :(

@winksaville
Copy link
Contributor Author

winksaville commented Mar 9, 2021

Travis finally finished, hopefully u can just kick bors :)

@eldruin
Copy link
Member

eldruin commented Mar 9, 2021

Yeah, let's try again...

bors retry

@bors
Copy link
Contributor

bors bot commented Mar 9, 2021

Build succeeded:

@bors bors bot merged commit 0da0f43 into rust-embedded:master Mar 9, 2021
bors bot added a commit that referenced this pull request Mar 9, 2021
309: Update 06 hello world to use shared cargo config r=eldruin a=winksaville

This is dependent on #308 but there is a reduction in number of lines for 06-hello-world, usually a positive :)


Co-authored-by: Wink Saville <wink@saville.com>
@winksaville winksaville deleted the Common-cargo-config.toml-and-openocd branch March 9, 2021 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-resources
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants