Skip to content

Conversation

@jayvdb
Copy link
Contributor

@jayvdb jayvdb commented Jul 31, 2021

#1058 depends on having a word dictionary, and the bionic dictionaries are missing words used in the specs that are in present in the GitHub ubuntu-latest workers used by https://github.com/swift-nav/libsbp/blob/641eda5/.github/workflows/generator.yaml#L20 .
The more recent the default wordlists used in the docker, the lower the chance that spec updates will require adding to the custom wordlist.

focal is the last ubuntu which includes https://packages.ubuntu.com/focal/clang-format-6.0 ; that could be sourced from somewhere else, but there are other issues to be solved moving to a release after focal. Not especially relevant at present, is this is also the last release which supports https://packages.ubuntu.com/focal/clang-6.0 . The portability project should mean that gcc-8 and beyond are usable with the current warning settings, so more flexibility should be coming soon.

Using groovy or hirsute instead of focal(LTS) will mean a less stable base, and requires an alternative for the deadsnakes ppa which only supports LTS.
c.f. https://launchpad.net/~deadsnakes/+archive/ubuntu/ppa & deadsnakes/issues#169

@jayvdb jayvdb force-pushed the jayvdb/docker-focal branch 2 times, most recently from d81ca7f to 1805bd4 Compare July 31, 2021 14:26
@jayvdb jayvdb marked this pull request as ready for review July 31, 2021 14:26
@jayvdb jayvdb force-pushed the jayvdb/docker-focal branch 3 times, most recently from 813ec72 to 4fbaaea Compare August 1, 2021 11:09
@jayvdb jayvdb requested a review from silverjam August 1, 2021 14:19
silverjam
silverjam previously approved these changes Aug 1, 2021
@silverjam
Copy link
Contributor

silverjam commented Aug 1, 2021

As long as this doesn't change the base set of tools and languages we support I suppose this is ok, but a bit concerned that originally the usage of bionic in docker (or at least my intent) was supposed to imply a check on the base Linux version we support. Bumping to focal means we don't have that sort of check anymore. It would be great if we could think of a way to mitigate this.

@silverjam silverjam dismissed their stale review August 1, 2021 16:24

Would like more discussion on alternatives before we go this route

@silverjam
Copy link
Contributor

As long as this doesn't change the base set of tools and languages we support I suppose this is ok, but a bit concerned that originally the usage of bionic in docker (or at least my intent) was supposed to imply a check on the base Linux version we support. Bumping to focal means we don't have that sort of check anymore. It would be great if we could think of a way to mitigate this.

One idea would be to just run the check in CI-- if focal is needed to run correctly locally-- let's just make a dockerfile for that.

Another idea would be to make the dockerfile support bionic or focal as a base.

@jayvdb jayvdb force-pushed the jayvdb/docker-focal branch from 805a34d to d9bfd9c Compare August 2, 2021 00:11
@jayvdb
Copy link
Contributor Author

jayvdb commented Aug 2, 2021

Another idea would be to make the dockerfile support bionic or focal as a base.

Done. The bionic image is 6.82GB, vs 7.56GB for focal. Should I add CI to test bionic as well?

In addition to the spelling database, having everyone use the focal LTS image will likely result in a better generated sbp.pdf.

@jayvdb jayvdb force-pushed the jayvdb/docker-focal branch 3 times, most recently from 3d57567 to 52b1c55 Compare August 3, 2021 09:50
@jayvdb jayvdb requested review from silverjam and triarius August 3, 2021 12:35
&& apt-get update \
&& apt-get install -y \
cmake
&& curl https://sh.rustup.rs -sSf | sh -s -- -y --default-toolchain stable --profile minimal --no-modify-path \
Copy link

Choose a reason for hiding this comment

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

Do we do anything to pin the version (as opposed to the channel) of the rust toolchain we use? It should be alright to compile older projects with newer compilers because of the edition guarantees, so I don't think this is critical.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This project seems to be ok with using stable-vs-nightly and edition as the only rustc versioning

Copy link

@triarius triarius left a comment

Choose a reason for hiding this comment

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

LGTM!

jayvdb added 10 commits August 4, 2021 14:16
pythonX.Y-dev are not needed due to removal of JIT.
Adds texlive-latex-extra as only needed recommendation.

Make stack dependencies libffi-dev, gnupg and netbase explicit
otherwise stack installs these using apt-get without
cleaning up.
Reduces the build time due to less fetches of the package lists,
and allows removal of many packages needed only for using apt.
Also support building groovy image, although
some components of sbp wont build due to
missing Python versions and wrong version of
clang-format.
@jayvdb jayvdb force-pushed the jayvdb/docker-focal branch from 52b1c55 to 5efc3c6 Compare August 4, 2021 06:17
@jayvdb jayvdb merged commit 5efc3c6 into master Aug 4, 2021
@jayvdb jayvdb deleted the jayvdb/docker-focal branch August 4, 2021 07:22
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.

4 participants