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

CI builds and Docker image cleanup #9

Merged
merged 7 commits into from
May 1, 2024
Merged

Conversation

darionhaase
Copy link
Member

This adds support for automatic builds using GitHub actions:

  • nightly (on each commit to the main branch), and
  • tagged releases on pushes of v<major>.<minor>.<patch> tags.

Tagged releases need to be published manually, while binaries for nightly-builds are immediately available.
The workflow builds binaries for macOS (Intel and M1), Windows, and Debian (rust target x86_64-unknown-linux-gnu) which are available as part of the GitHub release.

In addition, Docker images (amd64 and arm64) are built and published on GitHub Packages. As part of this, the Dockerfile has been cleaned up to support multi-platform builds. The final image builds upon a distroless image and only contains the caesar binary.
For tagged releases, the Docker images are only build after the release has been published on GitHub, to prevent premature access. As a result, Docker images of tagged releases will only be available with a small delay.

Copy link
Collaborator

@Philipp15b Philipp15b left a comment

Choose a reason for hiding this comment

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

Thanks! I've left some minor comments.

uses: ./.github/workflows/create_release.yml
with:
git_ref: ${{ github.ref_name }}
is_prerelease: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing empty line

- name: Log in to the Container registry
uses: docker/login-action@65b78e6e13532edd9afa3aa52ac7964289d1a9c1
with:
registry: ${{ env.REGISTRY }}
username: ${{ github.actor }}
password: ${{ secrets.GITHUB_TOKEN }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file has some trailing whitespace


### Installing Dependencies
### a) Pre-Built Binary
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit strange that "Option A" has sub-headings "a)" and "b)". Same for "Option B". Maybe just enumerate headings as "1." and "1.1."?

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed the existing documentation which was using A and B, so a) and b) were natural choices for subsections. But I also found this odd. Do you want to drop the "Option" prefix altogether or just "Option A"->"Option 1" etc.?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally, we make clear that these sections are all alternatives, not a sequence of steps to be done. Maybe prefix everything with "Option"? And have a sub-numbering?


2. Extract the `caesar` binary from the downloaded archive.

:::info Additional Steps
Copy link
Collaborator

Choose a reason for hiding this comment

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

This box visually very striking, but irrelevant for everyone not on MacOS. Just add a third step with a sentence just for MacOS.

Copy link
Member Author

@darionhaase darionhaase Apr 29, 2024

Choose a reason for hiding this comment

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

I agree that the color is forcing too much attention to the box. Maybe a :::note (grey) is better in this context?

I disagree on just adding it as a third step to the overall procedure, with the same reasons as in our earlier discussion. It is not a necessary step for every Mac user. It is merely a hint which might help with getting the tool running. As such I would prefer to keep it separate from the 'main instructions'. The current layout is also suitable to cover potential future hurdles on other systems.

  • One could remove the tabs for Debian/Linux and Windows, as they are just placeholders. My worry was that this causes confusion on the readers site, as the selected tab is synced on the whole page. A user might wonder whether their browser is malfunctioning and they are missing out on essential information.
  • Wrapping all steps in the platform-dependent tabs (with 1+2 being the same everywhere) would be an alternative but causes a lot of useless duplication.

I just tried the one-tab option again. And I think it looks better than expected (reasonable and not confusing). So I would propose using the single-tab option (as it effectively highlights that this is a macOS-specific step), together with the grey note admonition. Without the box, the indentation of the tab(s) seems off.


### b) Docker Image

We also provide a Docker image.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe mention that we provide Docker images for both x86-64 and ARM?

docker run -it -v $PWD/programs:/root/caesar/programs/ ghcr.io/moves-rwth/caesar:latest /root/caesar/programs/example.heyvl
```

## Option B: Compiling Locally
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this section is irrelevant to most people, could you add some vertical space before this heading? E.g. <div style="margin-top: 2em" />?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that there should be some space, but fail to see how this is related to this 'section being usually irrelevant'. I mean: it is a new section, you can clearly notice it by the bigger heading font. On my end it looks like there is not enough vspace due to the scrollbar above, but comparing with other pages of the documentation, the spacing is equivalent and I think a uniform design is good. Maybe one could/should handle this more generally in the site's CSS?

On a more general note: I find these straight-line (sub)subheadings without any indentation hard to navigate/group.

</TabItem>

1. [Follow the instructions](https://rustup.rs/) to install Rust via rustup.
2. Install Python, CMake and LLVM
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here, a link to the Chocolatey install website would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. Chocolatey is the package manager for Windows (winget does not seem to be as wide-spread yet), just like Homebrew is the package manager for macOS.

As these instructions are part of the DIY section, I find it reasonable to expect at least some prior familiarity with the de-facto standard tools on your platform of choice. Otherwise you would just use the default binaries.

(On a similar note: We also don't show how to install Docker, and rightfully so, as I do not think that this is in scope.)

@Philipp15b Philipp15b merged commit dc5a5a3 into moves-rwth:main May 1, 2024
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.

2 participants