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

Introduce a GitHub Actions workflow for publishing a release and fix npm publish warnings about the package.json format #18350

Merged
merged 2 commits into from
Jul 1, 2024

Conversation

timvandermeij
Copy link
Contributor

@timvandermeij timvandermeij commented Jun 30, 2024

The commit messages contain more details about the individual changes.

Notes:

  • This PR depends on Remove the on_release.js file botio-files-pdfjs#45 being merged and deployed to the bots first (to avoid publishing twice).
  • This PR depends on the NPM_TOKEN repository secret being configured in this repository, with the same token that the bots currently have.
  • This PR is made now in the hope that we can use the new flow for the upcoming release.
  • I have tested the workflow in my fork by making two small changes: adding the on: pull_request trigger so that I could trigger the workflow from my branch (because releases require it to be merged to master first which is inconvenient for testing) and adding the --dry-run option to the npm publish command so that we can see what e.g. the release tarball would contain without actually having to publish anything yet. You can find the logs of the dry-run for the first commit at https://github.com/timvandermeij/pdf.js/actions/runs/9732441891/job/26858099909?pr=11 and with the second commit applied at https://github.com/timvandermeij/pdf.js/actions/runs/9732541964/job/26858344999?pr=11 (showing that the warnings are now gone). Release tarball correctness can be verified by comparing the file listing in the logs to the file listing on https://www.npmjs.com/package/pdfjs-dist/v/latest?activeTab=code, and seeing that the "total files" property of 334 and the "unpacked size" property of 36 MB matches what's listed on NPM for the current release.
  • The Git publishing logic is removed because publishing to the pdfjs-dist repository has been broken for a long time and the current logic fails to run on GitHub Actions. To keep the scope of this patch small and to only port the logic we actually need and have functional right now I'll make a follow-up issue for revamping the Git publishing part in a separate scope (because that'll have to be re-implemented anyway).

Fixes a part of #11851.

This commit migrates this functionality away from the bots. Note that
the NPM token must be configured as a repository secret before this
workflow can execute.

The following resources are relevant for this patch:

- Publishing packages to the NPM registry:
  https://docs.github.com/en/actions/publishing-packages/publishing-nodejs-packages#publishing-packages-to-the-npm-registry
- Creating secrets for a repository:
  https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions#creating-secrets-for-a-repository
This commit removes the following warnings from the `npm publish` output:

```
npm warn publish npm auto-corrected some errors in your package.json when publishing.  Please run "npm pkg fix" to address these errors.
npm warn publish errors corrected:
npm warn publish Removed invalid "scripts"
npm warn publish "repository.url" was normalized to "git+https://github.com/mozilla/pdfjs-dist.git"
```

For the "scripts" section it turns out that if the package doesn't have
any scripts it's expected to explicitly set it to an empty object; refer
to npm/cli#6918 and
denoland/dnt#414.
Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@calixteman calixteman merged commit b5d554e into mozilla:master Jul 1, 2024
9 checks passed
@timvandermeij timvandermeij deleted the github-actions-release branch July 1, 2024 13:05
timvandermeij added a commit to timvandermeij/pdf.js that referenced this pull request Jul 13, 2024
The Git logic for pushing to the `pdfjs-dist` repository was already
removed in PR mozilla#18350, but it was forgotten to also remove the logic to
create a Git repository in the first place. This commit fixes the open
action for that from
mozilla#18358 (comment).

Moreover, we implement the suggestion from
mozilla#18358 (comment)
about moving the Git URL to a separate variable for consistency and we
update the homepage URL to the HTTPS scheme to avoid an HTTP -> HTTPS
redirect and enforce TLS-encrypted connections.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants