-
Notifications
You must be signed in to change notification settings - Fork 5
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
Conversation
Based on ripgrep/dafny workflows.
Keep the OOPSLA23 Dockerfile for use in interactive environments
There was a problem hiding this 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.
.github/workflows/tagged_release.yml
Outdated
uses: ./.github/workflows/create_release.yml | ||
with: | ||
git_ref: ${{ github.ref_name }} | ||
is_prerelease: false |
There was a problem hiding this comment.
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 }} | ||
|
||
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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."?
There was a problem hiding this comment.
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.?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" />
?
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
This adds support for automatic builds using GitHub actions:
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.