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

Compute $(STDLIB_MODULES) with GNU make functions instead of runtime/sak #12293

Merged
merged 1 commit into from
Jun 19, 2023

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Jun 7, 2023

This PR follows on from #12287 (comment).

stdlib/StdlibModules contains the macro $(STDLIB_MODULES) which expands to the actual module list for the Standard Library (i.e. with Stdlib__ prefixes). In #10169, this was computed using a series of shell calls which was hideously slow on Windows and which got changed in #10451 with the introduction of the sak tool.

Even ignoring questions of taste or otherwise about the use of C, this constitutes a slightly confusing state of affairs in the build system. $(STDLIB_MODULES) is evaluated, but it is the responsibility of the expansion site to ensure that the sak tool is compiled before this happens. In other words, the STDLIB_MODULES variable has prerequisites!

This isn't ideal, and means that Makefile.common contains some dark magic to ensure that sak is compiled before StdlibModules is included (which can also cause sak to be (re-)built when not really necessary).

This can be done fairly simply, if inelegantly, in GNU make itself. GNU make doesn't directly have a function to convert case, but it can efficiently evaluate 26 calls to $(patsubst ... - i.e. we can just do $(patsubst a%, A%, $(patsubst b%, B%, .... I'm not that keen on 27 right brackets in a row at the end of it, so instead the macro CAPITALIZE here receives two space-separated alphabets, and is defined recursively. I like this not involving any auxiliary script.

runtime/sak remains only for the purpose of formatting the location of the Standard Library, but this is used in a much more normal way to generate runtime/build_config.h (where runtime/sak is just a normal dependency). I think I have something in the freezer for that, too, but for another day.

@shindere
Copy link
Contributor

shindere commented Jun 15, 2023 via email

Copy link
Contributor

@nojb nojb left a comment

Choose a reason for hiding this comment

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

LGTM

Please leave the OCaml analog in; it helps understanding the macro for Make non-experts.

@dra27
Copy link
Member Author

dra27 commented Jun 17, 2023

Thanks, @nojb! @shindere and I ran out of steam in the heat yesterday, but there’s a slightly simpler version at dra27@e307048#diff-0ed518c14a9f4fdaa0f4d9fb9680bf7c1d4068015e46112d39e735c53cace4dfR107 (with a non-OCaml comment) - how does that look to you?

@nojb
Copy link
Contributor

nojb commented Jun 17, 2023

how does that look to you?

Also LGTM!

@dra27
Copy link
Member Author

dra27 commented Jun 17, 2023

Thanks! Revised version pushed here - I'll leave it for Monday when @shindere will have a chance to have a look.

@shindere
Copy link
Contributor

shindere commented Jun 19, 2023 via email

@shindere shindere merged commit 06d2fce into ocaml:trunk Jun 19, 2023
10 checks passed
@dra27 dra27 deleted the simpler-prefixing branch June 19, 2023 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants