-
Notifications
You must be signed in to change notification settings - Fork 693
Handle ragel better with meson #3383
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
base: main
Are you sure you want to change the base?
Conversation
9c9fb91 to
2f55154
Compare
.github/workflows/linux-ci.yml
Outdated
| - run: sudo pip3 install fonttools meson==0.56.0 | ||
| - name: run | ||
| run: meson build -Db_coverage=true --auto-features=enabled -Dgraphite=enabled -Dchafa=disabled -Dragel_subproject=true -Doptimization=2 | ||
| run: meson build -Db_coverage=true --auto-features=enabled -Dgraphite=enabled -Dchafa=disabled -Dragel=disabled -Doptimization=2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We no longer test the ragel subproject on ci builds, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The project options specify wrap_mode=nofallback
2f55154 to
66f009f
Compare
|
Is this ready to be merged? |
|
Why is the fedora bot failing? Should we instruct it to install ragel then? |
We shouldn’t need ragel on any CI jobs (unless we are specifically testing ragel generation, which i snot the case here). That being said, we do actually install ragel on this job but Fedora has the new ragel that we don’t want. This IMHO means the changes in this PR isn’t quite working yet because users shouldn’t need to do anything if they are simply building HarfBuzz and suitable ragel is not present. |
| ) | ||
| endforeach | ||
| message('You have to install ragel if you are going to develop HarfBuzz itself') | ||
| ragel = 'error' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this?
| ragel = 'error' | ||
| endif | ||
| ragel_helper = find_program('gen-ragel-artifacts.py') | ||
| foreach rl : hb_base_ragel_sources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this part should remain in the else case of if not has_ragel, that's what make the CI fail AFAIK.
|
Is someone going to finish this? |
|
I lost track of this PR (sorry!) but I am still interested in getting it to work. |
Any chance you can finish this? |
|
What's the status of this? |
Do not run ragel unless the rl sources have actually been edited. If they have been edited, error with an informative error message
…oject() It does automatic subproject fallback and simplifies the code.
66f009f to
e57c74e
Compare
|
Sorry I forgot about this again. :( I have just rebased it including some tweaks for the CI. |
|
Let's decide one way or another here. |
Just like autotools, ragel should only be required when rl sources are actually out of date.
Fixes #3378