Fix bootstrapping on Windows #892

minhqdao · 2023-04-26T12:15:37Z

Address #891.

Bootstrapping currently doesn't work on Windows using gfortran because the _WIN32 macro isn't exported by the compiler. This is an attempt to fix it by letting fpm export the FPM_IS_WINDOWS macro.

@zoziha @arteevraina @henilp105 @awvwgk

minhqdao · 2023-04-26T12:29:43Z

Well, I obviously have to fix the tests, but you could still check if it builds on Windows.

zoziha · 2023-04-26T12:54:21Z

Currently, manually declare the FPM_IS_WINDOWS macro with the following command, fpm compiles normally on Windows:

> fpm -v
Version:     0.8.1, alpha
***
OS Type:     Windows
> fpm build --flag "-DFPM_IS_WINDOWS"
***
install.f90                            done.
main.f90                               done.
libfpm.a                               done.
fpm.exe                                done.
[100%] Project compiled successfully.

minhqdao · 2023-04-26T14:45:27Z

Currently, manually declare the FPM_IS_WINDOWS macro with the following command, fpm compiles normally on Windows:

> fpm -v
Version:     0.8.1, alpha
***
OS Type:     Windows
> fpm build --flag "-DFPM_IS_WINDOWS"
***
install.f90                            done.
main.f90                               done.
libfpm.a                               done.
fpm.exe                                done.
[100%] Project compiled successfully.

Thanks for checking! That's a good start :)

minhqdao · 2023-04-27T06:14:02Z

Could you please try it again on Windows without the manual flag?

I still don't understand why the CI is still not getting it, though. I thought that the change should be registered in bootstrapping mode. 🤔

zoziha · 2023-04-27T13:18:28Z

I think if we don't use FPM_IS_WINDOWS, we will definitely fail to compile on Windows.
(CI.yml and CI status on Windows, gcc does not have realpath function on Windows?)

minhqdao · 2023-04-28T04:07:58Z

I think if we don't use FPM_IS_WINDOWS, we will definitely fail to compile on Windows.
(CI.yml and CI status on Windows, gcc does not have realpath function on Windows?)

In my understanding, bootstrapping means that we're compiling and running fpm without external dependencies. fpm adds the FPM_IS_WINDOWS macro to all the dependencies, therefore I'm surprised to see realpath being executed instead of fullpath.

zoziha · 2023-04-28T07:19:29Z

It seems that the fpm bootstrap in CI.yml looks like this:

Build fpm-0.8.0 with setup-fpm and compile the current commit fpm-latest.
Bootstrap with fpm-latest and test.

The first step needs to manually set the macro definition FPM_IS_WINDOWS. You might want to take a look at CI.yml.

minhqdao · 2023-04-28T07:27:41Z

It seems that the fpm bootstrap in CI.yml looks like this:

Build fpm-0.8.0 with setup-fpm and compile the current commit fpm-latest.

Bootstrap with fpm-latest and test.

The first step needs to manually set the macro definition FPM_IS_WINDOWS. You might want to take a look at CI.yml.

Ah, yes.

Ok, I just put back the C routine, which is used when we're not bootstrapping, and then we differentiate between Windows and Unix using FPM_IS_WINDOWS in bootstrap mode. I think it should all work now.

src/fpm.f90

src/fpm/manifest.f90

perazz · 2023-04-29T07:55:42Z

src/fpm/manifest.f90

        type(toml_table), allocatable :: table
        character(len=:), allocatable :: root
+        logical :: set_is_windows_macro = .true.


Please help me understand here. Are we adding the FPM_IS_WINDOWS macro by default to every package that fpm is going to ever build?

In the beginning, I added it to the root package only, but it didn't work (it wasn't registered during compilation). Maybe the reason is that the root package is also added to the dependency tree and that's the place where the macro matters. It worked when I exported it for every dependency in the dependency tree. I did so also because I didn't want to differentiate between the root package and all the other dependencies in the dependency tree.

If you have any better idea, let me know. I'll see if I can add the dependency to the dependency tree with the flag in place.

Yes, I had a look again. get_package_data is called on every package of the dependency tree (again) and that's where the macro needs to be set for the root package. So if I did this, it works:

if (i == 1) then call get_package_data(dependency, manifest, error, apply_defaults=.true., add_is_windows_macro=.true.) else call get_package_data(dependency, manifest, error, apply_defaults=.true., add_is_windows_macro=.false.) end if

So I'm open for better suggestions.

What we could do is add an opt-in instead of an opt-out. Meaning that we could add an export-windows-macro flag to the [build] config, for example, and set it to true in the fpm manifest:

[build] export-windows-macro = true

That way we won't "pollute" any procedures, and it will only registered for the root package. How do you feel about this solution? @awvwgk @perazz

My understanding is that minGW/MSYS compilers don't have access to realpath on Windows. So I would start looking for a kiss fix first. What would happen if we just extended this for macros _WIN64, __MINGW32__, __MSYS__, __MINGW64__:

fpm/src/fpm_os.c

Lines 11 to 15 in fc4ac0e

#ifndef _WIN32

return realpath(path, resolved_path);

#else

return _fullpath(resolved_path, path, maxLength);

#endif

It's probably _WIN64 what we're looking for:
https://github.com/Alexpux/mingw-w64/blob/d0d7f784833bbb0b2d279310ddc6afb52fe47a46/mingw-w64-headers/crt/_cygwin.h#L28-L34

That part of the code has so far not been a problem, but the bootstrapping is. If that part of the code you're referring to needs a fix, I'm happy to open another PR in the future.

@minhqdao @zoziha please take a look at this approach:

main...perazz:fpm:test_windows_macros

I can't test it on msys2 right now (the CI has minGW64) but I believe it should work.

@perazz The gfortran compiler only presets the macro definition __GFORTRAN__, other macro definitions such as _WIN32, _WIN64, __MINGW64__ are not preset.

What gfortran provider on Windows has this issue? I understood it is from MSYS, or is it another installation? If it is minGW or MSYS, then some of those macros should be defined I believe.

The previous Fortran standards did not specify preprocessors, and Fortran should not support preprocessors, but compiler manufacturers still adapted C preprocessors for Fortran. GCC (C/C++ language compiler) has these preset macro definitions, while gfortran does not, and I haven't seen any gfortran providers (msys, cygwin, mingw) on Windows preset macro definition _WIN32.

zoziha · 2023-04-30T07:48:08Z

@minhqdao The mingw-w64-fpm package in MSYS2 uses the single-source-file version, which does not use any C source files other than the C standard library, but only a single fpm-<latest>.F90 file.
So I tested the following command on Windows-MSYS2 with fpm-0.8.1:

> fpm build
[100%] Project compiled successfully.

> fpm build --flag "-DFPM_IS_WINDOWS"
[100%] Project compiled successfully.

> fpm build --flag "-DFPM_BOOTSTRAP"
fpm.exe                                failed.
[100%] Compiling...
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: build\gfortran_DE26B7BFDD5D0589\fpm\libfpm.a(src_fpm_os.F90.o):fpm_os.F90:(.text+0xf03): undefined reference to `realpath'
collect2.exe: error: ld returned 1 exit status
<ERROR> Compilation failed for object " fpm.exe "
<ERROR>stopping due to failed compilation
STOP 1

> fpm build --flag "-DFPM_BOOTSTRAP -DFPM_IS_WINDOWS"
[100%] Project compiled successfully.

Since the gfortran compiler predefines ~~_WIN32~~FPM_IS_WINDOWS, if we manually define ~~_WIN32~~FPM_IS_WINDOWS via fpm, this does seem to be a feature of fpm.

Should fpm predefine ~~_WIN32~~FPM_IS_WINDOWS, the default behavior or as an optional feature, maybe we have to discuss how to handle this? @minhqdao @perazz @awvwgk

minhqdao · 2023-04-30T07:56:21Z

Thanks for checking thoroughly, @zoziha. I once tried to let fpm export _WIN32 and it didn't work. Meaning that using gfortran, the _WIN32 could not be exported because gfortran would always override it with "not being exported". Maybe I did sth wrong back then, but that's also the reason this PR introduces FPM_IS_WINDOWS, sort of an "fpm way" of the _WIN32 macro.

It's strange that in MSYS2 the call to realpath is attempted. I can add checks for the other macros to see if it can be resolved that way.

zoziha · 2023-04-30T08:07:32Z

At the same time, I was wondering, as fpm becomes more available for OS operations since we start using C sources (e.g., ptycheck, filesystem_utilities, fpm_OS), do we still need to maintain the single-source-file? Or to what stage do we intend to support the single-source-file?

Links

awvwgk · 2023-04-30T08:13:47Z

The single source is crucial for bootstrapping, in most scenarios we cannot rely on having an fpm executable around for building unless we can provide another way to bootstrap fpm, e.g. via another build system.

minhqdao · 2023-04-30T09:57:47Z

@minhqdao The mingw-w64-fpm package in MSYS2 uses the single-source-file version, which does not use any C source files other than the C standard library, but only a single fpm-<latest>.F90 file. So I tested the following command on Windows-MSYS2 with fpm-0.8.1:
> fpm build
[100%] Project compiled successfully.

> fpm build --flag "-DFPM_IS_WINDOWS"
[100%] Project compiled successfully.

> fpm build --flag "-DFPM_BOOTSTRAP"
fpm.exe                                failed.
[100%] Compiling...
C:/msys64/mingw64/bin/../lib/gcc/x86_64-w64-mingw32/12.2.0/../../../../x86_64-w64-mingw32/bin/ld.exe: build\gfortran_DE26B7BFDD5D0589\fpm\libfpm.a(src_fpm_os.F90.o):fpm_os.F90:(.text+0xf03): undefined reference to `realpath'
collect2.exe: error: ld returned 1 exit status
<ERROR> Compilation failed for object " fpm.exe "
<ERROR>stopping due to failed compilation
STOP 1

> fpm build --flag "-DFPM_BOOTSTRAP -DFPM_IS_WINDOWS"
[100%] Project compiled successfully.
Since the gfortran compiler predefines ~~_WIN32~~FPM_IS_WINDOWS, if we manually define ~~_WIN32~~FPM_IS_WINDOWS via fpm, this does seem to be a feature of fpm.

Should fpm predefine ~~_WIN32~~FPM_IS_WINDOWS, the default behavior or as an optional feature, maybe we have to discuss how to handle this? @minhqdao @perazz @awvwgk

If you are using fpm build, that means that you are using the released version, right? So you aren't running the code with the changes made in this PR? 🤔 Could it be that that's the reason why the bootstrapping isn't working?

I'm running fpm install and then ~/.local/bin/fpm build on Mac to actually run my code. Maybe it works then for you as well.

minhqdao · 2023-04-30T10:00:05Z

I now moved the flag to:

[preprocessor]
[preprocessor.example]
export-windows-macro = true

The default is false, meaning that it will only be exported for the root package (fpm), not for all the others if they don't have the flag set to true.

I think that's better but we'd need another release for it to build.

awvwgk · 2023-04-30T17:52:31Z

I think we are derailing here a bit, the current solution does not look well designed to me. Let's step back and have another look on the problem, please.

We have two scenarios here:

bootstrapping without fpm from the single-source version, we expect this version to build cleanly with any Fortran compiler supporting sufficient standard features to build a minimal version of fpm
building fpm with another (minimal) version of fpm, we also expect this to build cleanly without any manual intervention

For (1) we should be able to find a solution using the FPM_BOOTSTRAP macro to guard any code path which requires fpm specific features (compiling and linking C code, exporting special macros, ...). For (2) we should design a general set of features to provide better support, having a key named export-windows-macro is not going to fit well in the design of the fpm manifest, same for a hard coded macro called FPM_IS_WINDOWS.

A better option might be to export this information for all platforms / architectures using FPM_PLATFORM / FPM_ARCH or more flexible via

[preprocessor.cpp]
macros = [
  "FPM_PLATFORM={platform}",
  "FPM_ARCH={arch}"
]

Note that we cannot rely on those guards in bootstrapping mode (1) because we cannot determine those in a reliable way without manual intervention. So those have to be used with care unless we find an alternative for our current bootstrapping strategy.

zoziha · 2023-05-03T08:29:30Z

src/fpm/manifest.f90

@@ -126,6 +126,8 @@ subroutine get_package_data(package, file, error, apply_defaults)
            end if
        end if

+        call add_fpm_is_windows_macro(package%preprocess)


I know cargo supports identifying the OS and selectively compiling the code, and it seems like fpm can do something similar with this PR. (fpm also knows the operating system type when it compiles the code.)

minhqdao · 2023-05-04T10:10:12Z

[preprocessor.cpp]
macros = [
  "FPM_PLATFORM={platform}",
  "FPM_ARCH={arch}"
]

For me, this solution does not look so much different from FPM_IS_WINDOWS, it just has more information. So if we don't want to rely on exporting macros, we should probably find another solution.

What I don't understand, however, is the "manual intervention" part. fpm is able to identify the OS and set macros before compiling src. In my understanding, we aren't manually intervening.

Considering the bootstrapping issue, we could also get the absolute path by changing directories and running getcwd/_getcwd. That's of course not a great solution but it for sure is going to work on all systems and we'd only do that in bootstrapping mode.

One could also use the command line interface, but I don't think that fullpath is an actual program on Windows.

minhqdao · 2023-05-05T07:53:46Z

Please check once again if bootstrapping works on Windows. I hope that does so we can close this chapter at least for a while. @zoziha

zoziha · 2023-05-05T09:13:00Z

Please check once again if bootstrapping works on Windows. I hope that does so we can close this chapter at least for a while. @zoziha

Thanks for your sharing @minhqdao . All the following commands have been successfully tested:

> fpm build --flag "-DFPM_BOOTSTRAP -DFPM_IS_WINDOWS"
> fpm build --flag "-DFPM_BOOTSTRAP"
> fpm build --flag "-DFPM_IS_WINDOWS"
> fpm build

src/fpm/manifest/preprocess.f90

minhqdao added 3 commits April 26, 2023 17:31

Fix typo

62da310

Add option to add macro to package

f78f61a

Make testable

a06dee5

minhqdao added 4 commits April 26, 2023 22:20

Fix allocation

c8d476a

Remove allocatable attribute

d97d240

Fix index

c483944

Export macro for all dependencies

e2bd9ba

minhqdao added 4 commits April 28, 2023 13:06

Always register when bool is injected

048995c

Use C routine again if not bootstrapping

fd70bdc

Try to fix test

b226ffb

Not append on unix

59aae56

minhqdao added 4 commits April 28, 2023 14:34

Remove unused variable

89d35d5

Add tests

86dedad

Remove duplications

d4f8194

Shorten

ef2b4c2

minhqdao requested review from awvwgk, perazz, henilp105, zoziha and arteevraina April 28, 2023 08:47

perazz mentioned this pull request Apr 28, 2023

fpm: update to 0.8.1 msys2/MINGW-packages#17028

Closed

awvwgk reviewed Apr 28, 2023

View reviewed changes

src/fpm.f90 Outdated Show resolved Hide resolved

Assign later

616a5dc

perazz reviewed Apr 29, 2023

View reviewed changes

src/fpm/manifest.f90 Outdated Show resolved Hide resolved

perazz reviewed Apr 29, 2023

View reviewed changes

Assign later

c457351

minhqdao added 3 commits April 30, 2023 15:42

Use flag in build config

4bdf117

Add flag to fpm manifest

1c55f49

Move to preprocessor table

189dc8a

Remove remaining keys from build config

75c19cb

zoziha reviewed May 3, 2023

View reviewed changes

minhqdao added 5 commits May 5, 2023 13:19

Use cd in bootstrap mode

5dda226

Not compile C code in bootstrap mode

c4cb688

Add some tests

2ff6924

Use get_current_directory

8b3f559

Check that we're still in the same directory

ca9eb39

zoziha approved these changes May 5, 2023

View reviewed changes

src/fpm/manifest/preprocess.f90 Outdated Show resolved Hide resolved

Remove redundant key

49a6503

minhqdao linked an issue May 7, 2023 that may be closed by this pull request

A compilation error occurred when fpm bootstrapping on Windows #891

Closed

minhqdao merged commit d9d93c2 into main May 7, 2023

minhqdao deleted the fix-cpp branch May 7, 2023 17:11

	#ifndef _WIN32
	return realpath(path, resolved_path);
	#else
	return _fullpath(resolved_path, path, maxLength);
	#endif

Fix bootstrapping on Windows #892

Fix bootstrapping on Windows #892

Uh oh!

Conversation

minhqdao commented Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minhqdao commented Apr 26, 2023

Uh oh!

zoziha commented Apr 26, 2023

Uh oh!

minhqdao commented Apr 26, 2023

Uh oh!

minhqdao commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoziha commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minhqdao commented Apr 28, 2023

Uh oh!

zoziha commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minhqdao commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

perazz Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minhqdao Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

minhqdao Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

minhqdao Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

perazz Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minhqdao Apr 29, 2023

Choose a reason for hiding this comment

Uh oh!

perazz Apr 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoziha Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

perazz Apr 30, 2023

Choose a reason for hiding this comment

Uh oh!

zoziha Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zoziha commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

minhqdao commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zoziha commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Links

Uh oh!

awvwgk commented Apr 30, 2023

Uh oh!

minhqdao commented Apr 30, 2023

Uh oh!

minhqdao commented Apr 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

minhqdao commented Apr 26, 2023 •

edited

Loading

minhqdao commented Apr 27, 2023 •

edited

Loading

zoziha commented Apr 27, 2023 •

edited

Loading

zoziha commented Apr 28, 2023 •

edited

Loading

minhqdao commented Apr 28, 2023 •

edited

Loading

perazz Apr 29, 2023 •

edited

Loading

minhqdao Apr 29, 2023 •

edited

Loading

perazz Apr 29, 2023 •

edited

Loading

perazz Apr 29, 2023 •

edited

Loading

zoziha Apr 30, 2023 •

edited

Loading

zoziha commented Apr 30, 2023 •

edited

Loading

minhqdao commented Apr 30, 2023 •

edited

Loading

zoziha commented Apr 30, 2023 •

edited

Loading

minhqdao commented Apr 30, 2023 •

edited

Loading

minhqdao commented May 4, 2023 •

edited

Loading