-
Notifications
You must be signed in to change notification settings - Fork 62
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
Conversation
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.
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
Thanks Jonathan for the heads up. What about (For the record, with this PR as it is now, |
I'm not sure $(shell pwd) works if you use 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. |
Thanks Jonathan! 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 :
Alternatively, the check in the HACL* Makefile at https://github.com/hacl-star/hacl-star/blob/b228390cd45329996e80380135d17410bc23e4a7/Makefile#L46 |
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! |
Thanks a lot Jonathan for merging! |
Some Makefiles were defining
KRML_HOME
with Cygwin paths. This PR fixes them to native Windows paths.