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

Makefiles: fix KRML_HOME to work with Cygwin #503

Merged
merged 1 commit into from
Dec 11, 2024
Merged

Conversation

tahina-pro
Copy link
Member

Some Makefiles were defining KRML_HOME with Cygwin paths. This PR fixes them to native Windows paths.

@tahina-pro tahina-pro requested a review from msprotz December 6, 2024 06:11
Copy link
Contributor

@msprotz msprotz left a comment

Choose a reason for hiding this comment

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

CURDIR is good but only supported by GNU Make, so if this Makefile doesn't do it already, I would suggest aborting in case someone is running e.g. BSD make

@tahina-pro
Copy link
Member Author

tahina-pro commented Dec 10, 2024

Thanks Jonathan for the heads up. What about $(shell pwd), as is currently done in master? Do any BSD variants of make support it? If so, then I'd rather switch back to $(shell pwd)

(For the record, with this PR as it is now, ./everest make and ./everest test successfully work on Windows+Cygwin with official opam 2.3 (and OCaml 5.2.1): https://github.com/project-everest/everest/actions/runs/12239757622 .)

@msprotz
Copy link
Contributor

msprotz commented Dec 10, 2024

I'm not sure $(shell pwd) works if you use make -C from a different directory. Can you confirm?

In general, I am against attempting to write portable Makefiles. It results in more complicated, less readable, less maintainable Makefiles, and we already assume GNU Make in other parts of Everest. Plus, most of our makefiles have implicit dependencies on GNU Make. So, your earlier solution was great, and I'm in support of it, just error out if you're running with GNU Make.

@tahina-pro
Copy link
Member Author

tahina-pro commented Dec 10, 2024

Thanks Jonathan!
Yes, I can confirm $(shell pwd) works well with make -C dir. (It is the case right now with Karamel master and ./everest make)

I didn't find any reliable way to check for GNU make other than renaming the Makefile into GNUmakefile, per https://www.gnu.org/software/make/manual/html_node/Makefile-Names.html :

The first name checked, GNUmakefile, is not recommended for most makefiles. You should use this name if you have a makefile that is specific to GNU make, and will not be understood by other versions of make. Other make programs look for makefile and Makefile, but not GNUmakefile.

Alternatively, the check in the HACL* Makefile at https://github.com/hacl-star/hacl-star/blob/b228390cd45329996e80380135d17410bc23e4a7/Makefile#L46
can rule out OSX Make. Would that be enough?

@msprotz msprotz merged commit fabde37 into master Dec 11, 2024
1 check passed
@msprotz msprotz deleted the _taramana_windows branch December 11, 2024 09:21
@msprotz
Copy link
Contributor

msprotz commented Dec 11, 2024

I think it's good as it is then. No need to waste your precious time with any of these details, you make no more assumptions than we already do in the existing build and other parts of Everest. Thank you for double-checking!

@tahina-pro
Copy link
Member Author

Thanks a lot Jonathan for merging!

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.

2 participants