Skip to content

Rework dist process and add minimal documentation #137

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

Merged
merged 2 commits into from
Jan 7, 2021

Conversation

silvanocerza
Copy link
Contributor

I reworked a bit the DistTasks.yml to make it project agnostic as much as possible.

The VERSION var can now be either nightly if the NIGHTLY env var is set to true, a Git tag if NIGHTLY is not set and a tag is found in the current commit, else snapshot is used. Ideally the last case handles a build done locally when checked out at an untagged commit. This change is also reflected in the nightly.yml workflow.

I added the TIMESTAMP_SHORT back in the checksum filename since that should be always present even in tagged releases. This is the way it is in the CLI right now at least.

@silvanocerza silvanocerza requested a review from per1234 January 4, 2021 15:00
@silvanocerza silvanocerza self-assigned this Jan 4, 2021
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I added the TIMESTAMP_SHORT back in the checksum filename since that should be always present even in tagged releases. This is the way it is in the CLI right now at least.

Why is it useful to have a timestamp in addition to the tag? The reason I'm questioning this is because I had the idea that people might want to be able to download the checksums for the releases just as they can currently do with the nightly. But having the timestamp in the filename will make that more difficult than simply being able to insert the version they want into the URL.

@silvanocerza
Copy link
Contributor Author

Why is it useful to have a timestamp in addition to the tag? The reason I'm questioning this is because I had the idea that people might want to be able to download the checksums for the releases just as they can currently do with the nightly. But having the timestamp in the filename will make that more difficult than simply being able to insert the version they want into the URL.

The CLI checksum file always contains the timestamp so I did the same to keep it compatible, same for nightlies.
At least according to the documentation, I just tried to download it but I can't seem to find the file.
I'd expect https://downloads.arduino.cc/arduino-cli/nightly/nightly-20210101-checksums.txt to work but it doesn't.

@per1234
Copy link
Contributor

per1234 commented Jan 5, 2021

The CLI checksum file always contains the timestamp so I did the same to keep it compatible, same for nightlies.

For the nightlies, it definitely makes sense because the timestamp is what we use as the unique identifier of nightlies. But for the releases we already have a unique identifier: the version, so I don't see that the timestamp adds any benefits, while also having a disadvantage.

I think it's good to maintain consistency between arduino-lint and Arduino CLI's infrastructure where possible, but I also see this as an opportunity to have a fresh start and make improvements on Arduino CLI's infrastructure, which can then be ported back to Arduino CLI. Arduino CLI's checksum system is already broken (see below), so it needs to be be changed anyway. That could be done at the same time as this new build system you designed is put in place.

I'd expect https://downloads.arduino.cc/arduino-cli/nightly/nightly-20210101-checksums.txt to work but it doesn't.

See ATL-794

@silvanocerza
Copy link
Contributor Author

The CLI checksum file always contains the timestamp so I did the same to keep it compatible, same for nightlies.

For the nightlies, it definitely makes sense because the timestamp is what we use as the unique identifier of nightlies. But for the releases we already have a unique identifier: the version, so I don't see that the timestamp adds any benefits, while also having a disadvantage.

I think it's good to maintain consistency between arduino-lint and Arduino CLI's infrastructure where possible, but I also see this as an opportunity to have a fresh start and make improvements on Arduino CLI's infrastructure, which can then be ported back to Arduino CLI. Arduino CLI's checksum system is already broken (see below), so it needs to be be changed anyway. That could be done at the same time as this new build system you designed is put in place.

I'd expect https://downloads.arduino.cc/arduino-cli/nightly/nightly-20210101-checksums.txt to work but it doesn't.

See ATL-794

Aha! Didn't know it was broken for the CLI, am all in to change it then.

If a nightly is being built the version string will be
"nightly-<timestamp>".
Else if a tag is set in the currently checked out commit that is used.
In all other cases the "snapshot" string is used, this last case handles
a local build started manually by a developer.
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

I ran the nightly and release workflows in my fork and everything worked perfectly. Great work Silvano!

@silvanocerza silvanocerza merged commit ff95327 into main Jan 7, 2021
@silvanocerza silvanocerza deleted the scerza/fix-dist-task branch January 7, 2021 09:27
@per1234 per1234 added topic: infrastructure Related to project infrastructure type: enhancement Proposed improvement labels Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: infrastructure Related to project infrastructure type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants