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

Update synthesis docs #454

Merged
merged 24 commits into from
Dec 18, 2023
Merged

Update synthesis docs #454

merged 24 commits into from
Dec 18, 2023

Conversation

lekcyjna123
Copy link
Contributor

I have found today that our synthesis page in documentation is a little bit out-of-date, so I updated it by adding information how to use our pre-build docker image.

@tilk
Copy link
Member

tilk commented Jul 28, 2023

Please, check the grammar first before submitting any English text in a PR.

docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
@lekcyjna123 lekcyjna123 mentioned this pull request Nov 19, 2023
Copy link
Member

@tilk tilk left a comment

Choose a reason for hiding this comment

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

Almost here.

docs/synthesis/Synthesis.md Outdated Show resolved Hide resolved
cd coreblocks
git submodule update --init --recursive
cd ..
sudo docker pull ghcr.io/kuznia-rdzeni/riscv-toolchain:2023.10.08_v
Copy link
Member

Choose a reason for hiding this comment

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

I suggest having a latest tag to avoid updating this doc on every docker change. This needs to be changed on other lines too.

Suggested change
sudo docker pull ghcr.io/kuznia-rdzeni/riscv-toolchain:2023.10.08_v
sudo docker pull ghcr.io/kuznia-rdzeni/riscv-toolchain:latest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But we don't use the tag latest.

sudo docker pull ghcr.io/kuznia-rdzeni/riscv-toolchain:latest

That command doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

No, we don't. Maybe we should? (That was the suggestion.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should have two sets of tags? One to use in pipelines to allow easy tracking of changes and second tag latest for the documentation? Or maybe we should use only the latest tag and use checksums in pipelines?

I don't want to use latest in pipelines, because that will cause some tricky errors (e.g. updating dockers in preparation for Dockerfile change, while some other review is being tested).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added latest tag to verilog and amaranth-synth packages. To add tag to the riscv-toolchain I have to download it first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

riscv-toolchain have been also tagged with latest

Copy link
Member

Choose a reason for hiding this comment

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

I don't want to use latest in pipelines, because that will cause some tricky errors (e.g. updating dockers in preparation for Dockerfile change, while some other review is being tested).

Workflows should reference concrete versions, latest should be for convenience only.

@lekcyjna123
Copy link
Contributor Author

@Kristopher38 Could you do a second review?

@tilk tilk added the documentation Improvements or additions to documentation label Dec 7, 2023
@tilk tilk merged commit 723072b into master Dec 18, 2023
8 checks passed
@tilk tilk deleted the lekcyjna/update-synthesis-doc branch December 18, 2023 09:53
github-actions bot pushed a commit that referenced this pull request Dec 18, 2023
Co-authored-by: Marek Materzok <[email protected]>
Co-authored-by: Kristopher38 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants