-
Notifications
You must be signed in to change notification settings - Fork 108
Initial implementation of get_os() #142
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
Conversation
This PR actually fails tests, but they look like they passed due to #143. |
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.
It looks good, I added a few suggestions.
Why are you concerned that preprocessing may not work?
@milancurcic the tests are currently failing, even though they are green. The macros do not work, if you click on the output when we just run |
The latest commit prints os type 1 (Linux) on all platforms. Ok, I think the macros do not work at all. I am going to use an alternative implementation using environment variables. |
How did you pass the macro to gfortran? as -DFMP_OS_xxxx? |
If it could help, fpp and macros seem to work when compiling manually: gfortran -cpp -D_WIN32 fpm.F90
$ ./a.out
Fortran Package Manager (fpm)
OS Type: 3 |
@jvdp1 the issue is that those macros do not seem to be defined by default on any platform with gfortran (and obviously we need to support all Fortran compilers down the road, each defining slightly different macros). The current solution as of (a1dc068) seems to work. It's obviously not super robust, but it will get us started, and we can design the correct full solution in #144. |
Everything works except Windows. It just occurred tome that if $HOME doesn't exist, then we can assume Windows. I am going to rework it. |
The problem now is that if a Windows user defines %HOME% for any reason, the function will fail. I think the original %HOMEPATH% approach is more robust, but you also need to test the value: call get_environment_variable("HOMEPATH", val, status=stat)
if (stat == 0 .and. val(1:7) == "\Users\") then
r = OS_WINDOWS
return
end if |
So when I first implemented $HOMEPATH for Windows, it failed: https://github.com/fortran-lang/fpm/runs/899185101, but then the later commit which didn't touch it, succeeded: https://github.com/fortran-lang/fpm/runs/899226753. This is infuriating, I spent hours yesterday due to this fragility of the GitHub Actions. I think they sometimes test a previous commit, even though they claim to test the latest commit. This is really bad from a reliability perspective. Anyway, the latest commit (only using I can put in back the $HOMEPATH if you prefer. This is all fragile and can easily break anyway, if somebody declares HOMEPATH on linux for example. |
Yes, but testing for the value ( |
Right. I did it as you suggested, but I just don't know which of the two is more robust (will succeed in more cases):
Because I do not know if |
I checked the CI manually, it still seems to work (assuming the CI tested the latest commit). But given the fact that the previous commit seems to work, then if the master fails after this is merged, we can always revert the latest commit. This is ready for a final review and merge as far as I am concerned. |
@milancurcic, @everythingfunctional is this ok to go in? I have another PR (#145) that depends on this. |
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 it's good to go, thanks @certik!
It might be difficult to get this working via pre-processor.