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

Automate checksum generation for standalone CLI #14081

Merged
merged 1 commit into from
Aug 1, 2024

Conversation

thecrypticace
Copy link
Contributor

@thecrypticace thecrypticace commented Jul 29, 2024

We've done this manually until now which is fine but not ideal so this PR automates it so it cannot be forgotten.

@RobinMalfait do you know of a good way to test this w/o creating a tag? Should I add a workflow_dispatch trigger to the prepare release workflow?

Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

@RobinMalfait do you know of a good way to test this w/o creating a tag? Should I add a workflow_dispatch trigger to the prepare release workflow?

Maybe it's fine to run npm run build locally to validate this. Otherwise the workflow_dispatch trigger makes sense IMO but if we want to run a full release we might need to make it a --dry-run somewhere.

The changes look good to me, though :)

'./tailwindcss-windows-x64.exe',
]

const __dirname = path.dirname(new URL(import.meta.url).pathname)
Copy link
Member

Choose a reason for hiding this comment

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

Learned today that this will fail on Windows and you're supposed to use the fileURLToPath helper 🙈 https://github.com/tailwindlabs/tailwindcss/pull/14065/files#diff-8cdc2dcef376d2bbc6b8c9029a227c928f2aaa0f460776ba24d9d3eba110aab1

Copy link
Member

Choose a reason for hiding this comment

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

Luckily, we don't have to worry about this for this particular case since it will be executed on a linux machine

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but we should use to fileURLToPath anyway just for consistency and what not

Copy link
Member

Choose a reason for hiding this comment

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

@RobinMalfait haha yep you are right. My intention of sharing this was more to share what I have just learned anyways.

But I think I agree with @thecrypticace that we should probably keep this consistent so that the next time someone copies from this script, they start with a better default and it won't bring surprises on Windows.

@RobinMalfait
Copy link
Member

Yeah I would go for a manual workflow_dispatch or even a temporary change in an action that is already running to verify that the script works.

It looks good to me, but my only fear is that the import and node: prefix might not work if we are running on an older node version when preparing releases 🤔

Another alternative is to try and setup act (https://github.com/nektos/act) but that might not be as straightforward because of the generated executables.

@philipp-spiess
Copy link
Member

It looks good to me, but my only fear is that the import and node: prefix might not work if we are running on an older node version when preparing releases 🤔

Seems like we use Node 20 for the releases as we also do for the CI: https://github.com/tailwindlabs/tailwindcss/blob/next/.github/workflows/release.yml#L17

@thecrypticace
Copy link
Contributor Author

thecrypticace commented Jul 30, 2024

Wrong branch: https://github.com/tailwindlabs/tailwindcss/blob/master/.github/workflows/prepare-release.yml#L25

We're using node 16 so… we need to update that if we can. 😱

@thecrypticace
Copy link
Contributor Author

Gonna merge this and give it a test. I think I need to create a dummy tag first though b/c if I don't it'll overwrite the existing release (which would be bad)

@thecrypticace thecrypticace merged commit 28bd90e into master Aug 1, 2024
13 checks passed
@thecrypticace thecrypticace deleted the feat/v3-checksum-generation branch August 1, 2024 14:02
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