Skip to content

Conversation

interkosmos
Copy link
Member

As noted in the source code and discussed in #144, the OS detection of get_os_type() in fpm/src/environment.f90 is not accurate. Please review this request, which checks environment variables and the existance of some files in order to determine the OS more precisely.

@LKedward
Copy link
Member

LKedward commented Sep 7, 2020

Thanks @interkosmos, this looks like an improvement. Do note that you'll need to update any functions that use get_os_type such as list_files in fpm.f90 to consider all different OS behaviours.

I'm hoping #155 will be merged soon, and this further increase the usage of get_os_type (mainly for filesystem/command_line interaction) so it may be best to wait for this to be merged?

I have no experience with Solaris or BSD so I don't know what quirks need to be specifically addressed, but we may run into the problem that we cannot test them in the CI.

@awvwgk
Copy link
Member

awvwgk commented Sep 7, 2020

@LKedward BSD could be tested using Cirrus CI, which provides FreeBSD images. Grouping it together with OSX and Linux will probably be fine for most use cases right now, to avoid another CI provider for this project.

Testing cygwin is not (easily) possible on the GH actions windows image as well, which means another CI provider would be required for this purpose (like Appveyor).

No idea where to find a CI solution for Solaris.

@interkosmos
Copy link
Member Author

In most cases, the behaviour of OS_LINUX, OS_MACOS, OS_CYGWIN, OS_SOLARIS, and OS_FREEBSD should be identical, if we stick to POSIX standards. I can’t say if there are any CI providers that feature OpenSolaris-based images (OpenIndiana, illumos, Tribblix, …).

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

This looks good. I'll let @LKedward merge #155 first, and then this.

!!
!! At first, the environment variable `OS` is checked, which is usually
!! found on Windows. Then, `OSTYPE` is read in and compared with common
!! names. If this fails too, check the existance of files that can be
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
!! names. If this fails too, check the existance of files that can be
!! names. If this fails too, check the existence of files that can be

Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Thank you!

@LKedward
Copy link
Member

@interkosmos, would you mind also updating the following uses of get_os_type() in order to prevent the select case statements from silently falling through?

select case (get_os_type())
case (OS_LINUX,OS_MACOS)
filesep = '/'
case (OS_WINDOWS)
filesep = '\'
end select

select case (get_os_type())
case (OS_LINUX,OS_MACOS)
call execute_command_line("mkdir -p " // dir , exitstat=stat)
write(*,*) "mkdir -p " // dir
case (OS_WINDOWS)
call execute_command_line("mkdir " // windows_path(dir), exitstat=stat)
write(*,*) "mkdir " // windows_path(dir)
end select

select case (get_os_type())
case (OS_LINUX)
call execute_command_line("ls " // dir // " > "//temp_file, &
exitstat=stat)
case (OS_MACOS)
call execute_command_line("ls " // dir // " > "//temp_file, &
exitstat=stat)
case (OS_WINDOWS)
call execute_command_line("dir /b " // windows_path(dir) // " > "//temp_file, &
exitstat=stat)
end select

select case (get_os_type())
case (OS_LINUX)
print *, "OS Type: Linux"
case (OS_MACOS)
print *, "OS Type: macOS"
case (OS_WINDOWS)
print *, "OS Type: Windows"
end select

@certik
Copy link
Member

certik commented Sep 14, 2020

The current failure is (on Windows):

Command line: gfortran -c -Jbuild\gfortran_debug\fpm -Ibuild\gfortran_debug\toml-f -Wall -Wextra -Wimplicit-interface -fPIC -fmax-errors=1 -g -fbounds-check -fcheck-array-temporaries -fbacktrace -o build/gfortran_debug/fpm/fpm_filesystem.o src\fpm_filesystem.f90
Exit code: 1
Stderr:
src\fpm_filesystem.f90:2:10:

     use :: fpm_environment, only: get_os_type, &
          1
Fatal Error: Can't open module file 'fpm_environment.mod' for reading at (1): No such file or directory
compilation terminated.

So that needs to be fixed before we can merge it.

@interkosmos
Copy link
Member Author

@certik: does this happend because fpm_filesystem.f90 is compiled before fpm_environment.f90 by fpm?

@LKedward
Copy link
Member

@interkosmos can you try reverting to the non colon form of the use statement - this may be a bug in Haskell fpm.

Copy link
Member

@LKedward LKedward left a comment

Choose a reason for hiding this comment

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

Great work @interkosmos. CI is now passing so this looks good to go! Only left one minor comment regarding a remaining colon use statement.

Copy link
Member

@certik certik left a comment

Choose a reason for hiding this comment

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

Looks good.

Next time I recommend to do changes like " -> ' (and white space formatting) in a separate pull request, as those are unrelated to the "OS type identification" improvement. It makes it easier to review of what actually changed.

@interkosmos
Copy link
Member Author

Please excuse the inconvenience.

@milancurcic milancurcic merged commit c492ac3 into fortran-lang:master Sep 15, 2020
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.

5 participants