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

Re-enable MacOS runner #7753

Merged
merged 1 commit into from
Mar 21, 2025
Merged

Re-enable MacOS runner #7753

merged 1 commit into from
Mar 21, 2025

Conversation

tkoeppe
Copy link
Contributor

@tkoeppe tkoeppe commented Mar 20, 2025

No description provided.

@tkoeppe tkoeppe requested a review from jensmaurer March 20, 2025 02:43
Copy link
Member

@jensmaurer jensmaurer left a comment

Choose a reason for hiding this comment

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

Rebased on main and testing.

The "gramwrite" spelling error was not flagged by LaTeX; strange. Is this ever invoked?

The commit message of the main change has a typo "exract".


\NewEnviron{simplebnf}{\begin{ncsimplebnf}%
\BODY%
\gramwrite{\string\begin{ncsimplebnf}\meaningbody\BODY\string\end{ncsimplebnf}}%
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
\gramwrite{\string\begin{ncsimplebnf}\meaningbody\BODY\string\end{ncsimplebnf}}%
\gramWrite{\string\begin{ncsimplebnf}\meaningbody\BODY\string\end{ncsimplebnf}}%

Copy link
Member

Choose a reason for hiding this comment

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

Yes, "simpebnf" is unused throughout the standard. Please remove.

(We only use "ncsimplebnf".)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I suppose we should rename ncsimplebnf and ncrebnf? We can drop the "nc" there if we are reasonably sure that bnf is the only environment we'll ever want extracted.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's plausible to assume we only ever want "bnf" to be extracted; everything else are either grammar quotes or non-C++ grammar things (such as regex).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see, makes sense. Yeah, in that case a global renaming would be plausible (but is by no means urgent or important).

@jensmaurer
Copy link
Member

"diffpdf" in "Appearance" mode shows no differences. Please merge after fixing the minor findings.

Copy link
Member

@jwakely jwakely left a comment

Choose a reason for hiding this comment

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

The check.yml commit has an odd summary: "MacOS Linux runner"

Was "Linux" meant to say something else?

@tkoeppe
Copy link
Contributor Author

tkoeppe commented Mar 21, 2025

@jwakely Yeah, sorry, the genesis of this PR is a bit chaotic. I'll apply some of the commits directly and repurpose/rewrite the runner based things appropriately. Thanks!

@tkoeppe tkoeppe changed the title Replace grammar extraction with "environ"-based approach Re-enable MacOS runner Mar 21, 2025
@tkoeppe
Copy link
Contributor Author

tkoeppe commented Mar 21, 2025

Was "Linux" meant to say something else?

(I deleted the wrong word. Initially the commit said "Update MacOS and Linux runners". I meant to delete "Linux", not "Update".)

The path of the `sed` binary on MacOS has changed going from
Intel (macos-13) to M1 (macos-15).
@tkoeppe tkoeppe merged commit 9c3c97c into cplusplus:main Mar 21, 2025
2 checks passed
@tkoeppe tkoeppe deleted the mac_tex_cfg branch March 21, 2025 12:26
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