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

[CI] Add workflow to test ragel generation #2859

Merged
merged 2 commits into from Apr 16, 2022

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Apr 15, 2022

Description

Two files are generated with ragel. These files may not match the source files. Add a workflow to test, and save CI generated files if they do not match what is in the repo.

PR opened as a draft with existing workflows disabled, and tests are failing. See https://github.com/puma/puma/actions/runs/2173139081

Note that all of the puma workflows have 'workflow_dispatch' enabled, so they can be run at anytime on any branch by a member.

Closes #2858

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@MSP-Greg MSP-Greg marked this pull request as ready for review April 15, 2022 15:18
@dentarg
Copy link
Member

dentarg commented Apr 15, 2022

What was a7f4849?

@MSP-Greg
Copy link
Member Author

@dentarg

What was a7f4849?

Not sure. I suspect @nateberkopec did it locally, not sure why the output he got differs from the output on Actions?

All the Actions OS's are using ragel 6.10.

If you have time, can you check locally and see what your system(s) show?

@nateberkopec
Copy link
Member

Oh weird. Well I can always revert if something funky is going on locally for me!

@nateberkopec
Copy link
Member

You're a wizard, @MSP-Greg !

@nateberkopec nateberkopec merged commit a4b5c2f into puma:master Apr 16, 2022
@MSP-Greg
Copy link
Member Author

You're a wizard

We'll just say I'm persistent, know what Actions can do, and know that Powershell is the easiest way to run cross-platform scripts.

Just in case people haven't looked at the Actions pages, if you look at https://github.com/puma/puma/actions/runs/2173139081 in a desktop browser, the bottom of the page is 'Artifacts'. These are zip files of the ragel generated code, and are only created if the tests fail. Hence, if things get out of sync, download them, and a PR/commit should fix things.

Lastly, Actions workflows can be setup to only run if certain files change. I haven't worked with it much, not sure how it works if multiple commits are pushed, etc. I may experiment with it.

Remember that the ragel workflow (and the others also) can be run manually, one selects which branch to run the workflow on.
The UX is a bit odd, one has to go to the repo's main Actions page, then select a workflow on the left. Should see a box with 'This workflow has a workflow_dispatch event trigger.' and a button on the right. That button runs the workflow...

@nateberkopec

if something funky is going on locally

What version of ragel generated the files? I'd really like to see if we can figure why the files don't match.

@MSP-Greg MSP-Greg deleted the 00-ragel-test branch April 16, 2022 15:36
@dentarg
Copy link
Member

dentarg commented Apr 16, 2022

I'm also using ragel 6.10 (from Homebrew on macOS 12.3.1)

$ ragel --version
Ragel State Machine Compiler version 6.10 March 2017
Copyright (c) 2001-2009 by Adrian Thurston

I can't reproduce a7f4849 (not on current master a4b5c2f nor 1cea90a (the commit before regenerate ragel))

$ rm ext/puma_http11/http11_parser.c

$ git status --short
 D ext/puma_http11/http11_parser.c

$ bundle exec rake --trace ragel
** Invoke ragel (first_time)
** Invoke ext/puma_http11/http11_parser.c (first_time)
** Invoke ext/puma_http11/http11_parser.rl (first_time, not_needed)
** Execute ext/puma_http11/http11_parser.c
ragel ext/puma_http11/http11_parser.rl -C -G2 -I ext/puma_http11 -o ext/puma_http11/http11_parser.c
** Invoke ext/puma_http11/org/jruby/puma/Http11Parser.java (first_time, not_needed)
** Invoke ext/puma_http11/http11_parser.java.rl (first_time, not_needed)
** Execute ragel

$ git status
On branch master
Your branch is up to date with 'dentarg/master'.

nothing to commit, working tree clean

@MSP-Greg
Copy link
Member Author

@dentarg Thanks for checking.

JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
* [CI] Add ragel test for file changes

* Revert 'regenerate ragel' a7f4849
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.

New build step for verifying ragel
3 participants