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

[sc-24168] Add cargo-dylint #9

Conversation

frondeus
Copy link
Contributor

@frondeus frondeus commented May 7, 2024

cargo-dylint is a tool for compiling, linking and running dynamically linked, custom lints in our workspace.

Tested on:

  • My machine cross compiling from aarch64 to x86
    • Not only tested ./build.sh but also I put .zip in the docker-image-dev, and used impero-dev:dev in ./tools/do_bash.sh to run cargo dylint --all - with a success.
  • Jonathan's machine cross compiling from x86 to aarch64
  • This CI workflow, compiling all targets.

@frondeus frondeus requested review from arnodb and jgimpero May 7, 2024 10:38
@arnodb
Copy link
Contributor

arnodb commented May 7, 2024

Looks good to me but I'd like the thing to be tested before you tag tools and modify docker-image-dev. Unfortunately this is the least documented procedure.

First you need to build an x86_64.zip (or the one for your arch).

Then you need to copy it in your working copy of docker-image-dev.

Then you need to patch the Dockerfile for testing purpose only:

--- a/Dockerfile
+++ b/Dockerfile
@@ -55,12 +55,10 @@ ENV RUSTUP_HOME=/root/.rustup
 WORKDIR /usr/src/impero
 RUN curl https://sh.rustup.rs -sSf \
     | sh -s -- --profile minimal --default-toolchain "$RUST_TOOLCHAIN" -y
+Copy x86_64.zip x86_64.zip
 # Fetch tools needed for development
 RUN ARCH="$(uname -m)"; \
-    TAG="5.1"; \
-    URL="https://github.com/impero-com/docker-image-tools/releases/download/$TAG/$ARCH.zip"; \
     MOLD_VERSION="1.4.1"; \
-    curl -OL -H 'Accept: application/octet-stream' $URL && \
     unzip $ARCH.zip && rm $ARCH.zip && \
     chmod -R +x $ARCH && mv $ARCH/* $HOME/.cargo/bin/ && rm -r $ARCH && \
     curl -OL "https://github.com/rui314/mold/releases/download/v$MOLD_VERSION/mold-$MOLD_VERSION-$ARCH-linux.tar.gz" && \

Then you need to build the image on your machine.

And then you need to test by patching the impero docker-compose.yml file (change the image tag).

Once happy:

  • don't forget to revert dev changes
  • merge tools, tag tools, wait...
  • bump TAG in docker-image-dev
  • don't forget to actually test the presence of the new tools (see end of Dockerfile of docker-image-dev)
  • open PR, merge, tag, wait...
  • use the image tag in docker-compose.yml and in all workflows of impero

@frondeus frondeus marked this pull request as draft May 8, 2024 10:47
@frondeus frondeus force-pushed the wojciechpolak/sc-24168/improve-collections-serialization-in-get branch from 803fbdb to e4c84ad Compare May 8, 2024 12:35
@frondeus frondeus marked this pull request as ready for review May 8, 2024 13:00
@frondeus frondeus requested a review from Emm May 8, 2024 13:01
file /tmp/x86_64-unknown-linux-gnu/penguin
/tmp/x86_64-unknown-linux-gnu/sccache --help
/tmp/x86_64-unknown-linux-gnu/diesel --help
/tmp/x86_64-unknown-linux-gnu/diesel_ext --help
/tmp/x86_64-unknown-linux-gnu/cargo-audit --help
/tmp/x86_64-unknown-linux-gnu/cargo-watch --help
RUSTUP_TOOLCHAIN="stable" /tmp/x86_64-unknown-linux-gnu/cargo-dylint --help
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 is on purpose - dylint tools relies on rustup (even for --help, I made a ticket in GH for that), and since we don't have rustup in the worker (we do have inside of a docker), we can fake it by setting this env variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@frondeus frondeus force-pushed the wojciechpolak/sc-24168/improve-collections-serialization-in-get branch from e4c84ad to 7c37ce6 Compare May 10, 2024 07:33
@frondeus frondeus merged commit 962265f into master May 10, 2024
4 checks passed
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