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

tools: add WPT updater for specific subsystems #54460

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mertcanaltin
Copy link
Member

@mertcanaltin mertcanaltin commented Aug 20, 2024

a WPT updater that specifically targets the url subsystem for more controlled and focused updates.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/actions

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Aug 20, 2024
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Show resolved Hide resolved
@mertcanaltin
Copy link
Member Author

thanks for your suggestions

Copy link
Member

@RedYetiDev RedYetiDev left a comment

Choose a reason for hiding this comment

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

Here are my opinions

.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
@mertcanaltin mertcanaltin force-pushed the dev-wpt-test-updater branch 2 times, most recently from 616d720 to 9b1cda3 Compare August 20, 2024 12:55
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
@mertcanaltin mertcanaltin changed the title ci: add WPT updater tools: add WPT updater Aug 20, 2024
Copy link
Member

@RedYetiDev RedYetiDev left a comment

Choose a reason for hiding this comment

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

I also feel like this could be moved to a dep_updater file.

.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
.github/workflows/update-wpt.yml Outdated Show resolved Hide resolved
@RedYetiDev
Copy link
Member

By the way, I'm trying to update the WPT files in #54468.

IMO this could be done via

git node wpt <...>

Because the README.md file also needs to be updated.

@mertcanaltin
Copy link
Member Author

By the way, I'm trying to update the WPT files in #54468.

IMO this could be done via

git node wpt <...>

Because the README.md file also needs to be updated.

Do you have a suggestion, would that be enough for this?

- Name: Update README.md file
        Run: |
            # Update the README.md file with the new WPT version
            # Assuming you have a script or command to do this
            ./scripts/update-readme.sh ‘${{{ env.NEW_VERSION }}’

      - Name: Commit to Changes
        Run: |
            git config --global user.name ‘Node.js GitHub Bot’
            git config --global user.email ‘[email protected]’
            git add test/fixtures/wpt README.md
            git commit -m ‘test: Update WPT to ${{{ env.NEW_VERSION }} and update README.md’

@RedYetiDev
Copy link
Member

In my opinion, the entire WPT update process could be moved to a dep-updater script, using git node for each section. But, there are a lot of cases where updating these tests cause problems (just look at the errors in my PR), so maybe keeping it manual is good?

I'm not sure.

panva
panva previously requested changes Aug 26, 2024
Copy link
Member

@panva panva left a comment

Choose a reason for hiding this comment

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

In general I don't think updating WPTs automatically is a good idea. I can't find the issue now but I believe we've already had that discussion in the past, that's not to say we can't have it again... So:

  • The way to update WPTs is through the git node wpt ... command from node-core-utils, not checking out the WPT repo and copying over files
  • WPTs have dependencies in resources, interfaces, or generally a new folder that is not part of the checkout and requires manual intervention
  • Pulling in new WPTs often makes new WPTs fail, that also requires manual intervention
  • There are tentative WPTs that are not tied to a finished spec change, that will fail too
  • If there was an automated process then WPTs should be updated by their subsystem, not all at once, but that again raises an issue when that subsystem WPT change requires to pull in a file from a different directory that also needs to be updated
  • Some WPT subsystems currently cannot be updated (e.g. WebCryptoAPI) because we don't have a way to conditionally mark tests as failing depending on the system they run on, in WebCryptoAPI's case - test: update wpt test for WebCryptoAPI #54127, test,crypto: update WebCryptoAPI WPT #52222 (comment), RHEL's linked OpenSSL makes some vectors pass that don't pass anywhere else.

In my opinion a sensible way to update WPTs would be to pull a WPT subsystem from upstream (using git node wpt ...), run it, if it passes - open a PR, if it fails - have a keepalive issue where pending WPT updates are tracked.

FWIW this could also be hooked into the daily wpt job that runs the most up to date WPT checkout for the purpose of uploading the current state to wpt.io.

Remember that WPTs may very well fail for a long time when they are a) tentative or b) require a semver-major change to pass.

@panva
Copy link
Member

panva commented Aug 26, 2024

cc @nodejs/web-standards

@legendecas
Copy link
Member

yeah, we had discussion at #50567.

@mertcanaltin
Copy link
Member Author

I'm closing this place, thank you very much for the comments

@panva
Copy link
Member

panva commented Sep 21, 2024

mertcanaltin/node-auto-test/actions/runs/10972255885/job/30468306292 I did a test run here and it seems to give an error to the github token

I think it seems to suggest the script is ending prematurely. i.e. not syntactically correct.

@mertcanaltin
Copy link
Member Author

Passed 🎉 https://github.com/mertcanaltin/node-auto-test/actions/runs/10972357345/job/30468531837

The issue was that the if block wasn’t properly closed. I just added the missing fi at the end, and now everything works smoothly.

@panva
Copy link
Member

panva commented Sep 21, 2024

Passed 🎉 mertcanaltin/node-auto-test/actions/runs/10972357345/job/30468531837

The issue was that the if block wasn’t properly closed. I just added the missing fi at the end, and now everything works smoothly.

Can you run dispatch with two subsystems? url and WebCryptoAPI. The latter should open a PR.

@mertcanaltin
Copy link
Member Author

@panva
Copy link
Member

panva commented Sep 21, 2024

https://github.com/mertcanaltin/node-auto-test/actions/runs/10972538059/job/30468925665#step:4:7

It would appear @node-core/utils needs its config file with credentials to run this. Maybe an empty config file would do? I dunno, I'm short on time to look into this.

@RedYetiDev
Copy link
Member

You could also move all this logic into a dependency updater script, and call it like the rest of the updaters.

@mertcanaltin
Copy link
Member Author

You could also move all this logic into a dependency updater script, and call it like the rest of the updaters.

ı applied the latest updates, thank you

@mertcanaltin
Copy link
Member Author

You could also move all this logic into a dependency updater script, and call it like the rest of the updaters.

node/.github/workflows/update-openssl.yml

I looked here as an example

@panva
Copy link
Member

panva commented Sep 21, 2024

Look at .github/workflows/auto-start-ci.yml for Setup @node-core/utils

@panva
Copy link
Member

panva commented Sep 28, 2024

I believe the last 2 points weren't addressed. ncu is not configured and the new version variable is used before it is set

@mertcanaltin
Copy link
Member Author

I believe the last 2 points weren't addressed. ncu is not configured and the new version variable is used before it is set

Thanks a lot for your detailed feedback! I’ve now addressed the last two points you mentioned:

I've added a step to configure ncu (node-core-utils) with the necessary GitHub token.
I moved the calculation of the new_version earlier in the workflow to ensure it's set before being referenced.

@panva
Copy link
Member

panva commented Sep 28, 2024

@nodejs/actions please review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants