Skip to content

Conversation

jvaltin
Copy link
Contributor

@jvaltin jvaltin commented Aug 24, 2018

This Docker configuration builds the mcu firmware with Debian stable.

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 24, 2018

It should also be possible to integrate building with Docker into the current .travis.yml.

Alternatively, it would be possible to port the steps in the current .travis.yml into a series of Docker commands for the Travis' Docker CI environment.

Copy link

@x1ddos x1ddos left a comment

Choose a reason for hiding this comment

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

Why is there separate docker file for travis vs the regular builds? I would have expected the same docker file to be used for both dev, release and ci.

In other words, I would've expected the same methods to be used throughout all the build flows, plus scripts/ci.sh to run tests.

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 24, 2018

Why is there separate docker file for travis vs the regular builds? I would have expected the same docker file to be used for both dev, release and ci.

There is only one Docker configuration file in the repository. Travis does not currently use Docker. I agree that it could and would be reasonable for it to be used for dev, release, and ci. If that is the case, I would strongly suggest that we use debuerreotype as a base image and that we publish it.

The Docker file is intended as a functional replacement for the Vagrant setup. I found the current Vagrant configuration to be non-functional. The current vagrant up is fragile and appears to be currently broken due to changes in the Ubuntu packaging life cycle. It seemed positive to add a Dockerfile that produces a build and that is functional. I would be willing to also fix the vagrant build process though my understanding is that we cannot easily use vagrant in Travis unless we involve another service provider.

In other words, I would've expected the same methods to be used throughout all the build flows, plus scripts/ci.sh to run tests.

That isn't the case at the moment and this commit could be a step in the direction of a fully unified environment for dev, release, and ci with Docker.

@x1ddos
Copy link

x1ddos commented Aug 25, 2018

The Docker file is intended as a functional replacement for the Vagrant setup. I found the current Vagrant configuration to be non-functional

Then I would suggest the commit message and this pull request to be updated to reflect the reality. Otherwise, it's unclear what it's for.

Copy link

@x1ddos x1ddos left a comment

Choose a reason for hiding this comment

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

Travis does not currently use Docker

then why is the dockerfile has .travis extension? if/when this dockerfile is suitable for travis ci, it can be renamed at that time.

Also, what jv.gpg has to do with dockerfile.travis? seems like nothing in common and should be added in a separate pull request.

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 25, 2018

The PGP public keys of the contributors can be found in contrib/contributors_gpg_keys. Please add your PGP key with your first pull request.

I might have misunderstood the instructions in the README.md file. It says "The PGP public keys of the contributors can be found in contrib/contributors_gpg_keys. Please add your PGP key with your first pull request." This is my first pull request for the mcu repository. Is that instruction incorrect? I am happy to add it as a separate commit or a different pull request, if that documentation is out of date.

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 25, 2018

then why is the dockerfile has .travis extension? if/when this dockerfile is suitable for travis ci, it can be renamed at that time.

I don't think the name causes travis to use it, does it? I could rename it to Dockerfile.dev or something similar, do you have a suggestion for what would be a reasonable file extension that would not be confusing?

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 25, 2018

Then I would suggest the commit message and this pull request to be updated to reflect the reality. Otherwise, it's unclear what it's for.

Sure, I'll revise some details and push a new revision in the next few days.

@x1ddos
Copy link

x1ddos commented Aug 25, 2018

I might have misunderstood the instructions in the README.md file. It says "The PGP public keys of the contributors can be found in contrib/contributors_gpg_keys. Please add your PGP key with your first pull request."

Ah, I might've misunderstood too. Up to the repo owners, I guess.

I don't think the name causes travis to use it, does it? I could rename it to Dockerfile.dev or something similar, do you have a suggestion

Well, I causes some unclear cognitive activity as to why it's called .travis but there isn't a travis ci config, imho. A simple and standard Dockerfile would do, containing comments which explain what it's for.

Anyway, my 2 cents.

This commit adds a Docker.dev file which is an alternative to the Vagrant build
progress.  It is based on Debian stable and may be used as a basis for
building, and testing in a variety of environments where Vagrant may not
function.
@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 26, 2018

Ah, I might've misunderstood too. Up to the repo owners, I guess.

I think it makes sense to put the key into a different commit. I added it as a second commit. I suppose if there was an order that matters, it is now backwards. I am willing to reorder it or squash, if it matters.

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 26, 2018

Well, I causes some unclear cognitive activity as to why it's called .travis but there isn't a travis ci config, imho. A simple and standard Dockerfile would do, containing comments which explain what it's for.

Understood. I've updated the commits and I've named it Docker.dev. I will wait until I hear from @douglasbakkum before I working on any changes to Travis related build processes. It isn't needed and any local tests can easily go into Docker.dev as a way to ensure the local build works. That Docker file could also be added to Travis with less effort than porting the Travis build matrix into a Docker file. Travis should at least build the Docker image, so it won't become broken or stale as Vagrant appears to be for me.

@douglasbakkum
Copy link
Member

Thanks!

Vagrant has given me some headaches in the past too, so I am also inclined to change to Docker for deterministic builds. In this case, we can remove Vagrant instructions from README.md. (For past builds, Vagrant instructions are in the release notes.)

...this commit could be a step in the direction of a fully unified environment for dev, release, and ci with Docker.

Agree this would be nice. (The .travis naming was also confusing to me.)

I might have misunderstood the instructions in the README.md file. It says "The PGP public keys of the contributors can be found in contrib/contributors_gpg_keys. Please add your PGP key with your first pull request."

Not sure there is a functional difference. But yea, probably cleaner to add the key in a separate commit / PR. I believe order is important if separate PRs.

@jvaltin
Copy link
Contributor Author

jvaltin commented Aug 27, 2018

Vagrant has given me some headaches in the past too, so I am also inclined to change to Docker for deterministic builds. In this case, we can remove Vagrant instructions from README.md. (For past builds, Vagrant instructions are in the release notes.)

I would propose that we fix Vagrant and support both. It is still relevant for deployed firmware. I also want to check if we produce identical builds from Vagrant and Docker, for example.

Not sure there is a functional difference. But yea, probably cleaner to add the key in a separate commit / PR. I believe order is important if separate PRs.

I'll plan to squash it into a single commit or to make the gpg key the first commit.

@douglasbakkum
Copy link
Member

Tested and looks good!

One last nit:

It is probably better to specifically specify the gcc-arm-none-eabi version for deterministic building. The Debian Stretch gcc-arm-none-eabi version is 5.4.1, while the latest release is 7.3.1 (and past firmware releases have used v6.x.x). https://packages.debian.org/stretch/gcc-arm-none-eabi

Seems v7 may not be available via apt but can use the wget alternative used in the now deleted Vagrantfile.

@jvaltin
Copy link
Contributor Author

jvaltin commented Sep 7, 2018 via email

@douglasbakkum
Copy link
Member

As Vagrant is deleted in this PR, we will need a new deterministic build for the next release. I'll merge and then we can add such a build in a new PR.

Some questions arise in any case: Do we need v7 here? Do we want to ensure
we test builds as well as testing a specific release process? Is using
older compilers undesired?

I've noticed that newer GCC compilers catch more warnings, so potentially will catch more bugs.

@jvaltin
Copy link
Contributor Author

jvaltin commented Sep 7, 2018 via email

@douglasbakkum douglasbakkum merged commit c6e3829 into BitBoxSwiss:master Sep 7, 2018
douglasbakkum pushed a commit that referenced this pull request Sep 7, 2018
c6e3829 Add Docker tests file and remove Vagrant (Jakob Valtin)
23f7a35 Add my public key (Jakob Valtin)
9674c58 Add Docker config for building with Debian Stable (Jakob Valtin)
@douglasbakkum
Copy link
Member

I haven't dug into depth on Gitian but @jonasschnelli often recommends it. Now seems like a good opportunity to try it.

Definitely would be nice including more of clang's features in the build / CI (also ASAN, fuzzing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants