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

fix miniupnpc build on Windows by not escaping PATH #80

Merged
merged 1 commit into from
Mar 27, 2024

Conversation

jakubgs
Copy link
Member

@jakubgs jakubgs commented Mar 26, 2024

Also use correct ; divider. Otherwise the build fails with:

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-libp2p/repo (nim-libp2p-auto-bump-unstable)
$ make libminiupnpc.a --debug
Reading makefiles...
Updating makefiles....
Updating goal targets....
 File 'libminiupnpc.a' does not exist.
   File 'sanity-checks' does not exist.
  Must remake target 'sanity-checks'.
  Successfully remade target file 'sanity-checks'.
Must remake target 'libminiupnpc.a'.
process_begin: CreateProcess(NULL, git rev-parse --short HEAD, ...) failed.
Makefile.mingw:56: pipe: No error
gcc: fatal error: cannot execute 'cc1': CreateProcess: No such file or directory
compilation terminated.
make[1]: *** [Makefile.mingw:121: wingenminiupnpcstrings.exe] Error 1
make: *** [vendor/nimbus-build-system/makefiles/targets.mk:134: libminiupnpc.a] Error 2

Resolves:

I still don't get why this PATH modification is even necessary, it works without it.

@@ -132,7 +132,7 @@ nat-libs: | libminiupnpc.a libnatpmp.a
libminiupnpc.a: | sanity-checks
ifeq ($(OS), Windows_NT)
+ [ -e vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc/$@ ] || \
PATH=".:${PATH}" "$(MAKE)" -C vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc -f Makefile.mingw CC=$(CC) $@ $(HANDLE_OUTPUT)
"$(MAKE)" -C vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc -f Makefile.mingw CC=$(CC) $@ $(HANDLE_OUTPUT)
Copy link
Member

Choose a reason for hiding this comment

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

any idea why it fails? ie it seems strange that adding the current folder to the path would make it fail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Your guess is as good as mine, look at this. Here's without the change:

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-unstable/repo (unstable)
$ rm -f vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc/{libminiupnpc.a,wingenminiupnpcstrings.exe}

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-unstable/repo (unstable)
$ make libminiupnpc.a V=3
which gcc &>/dev/null || { echo "C compiler (gcc) not installed. Aborting."; exit 1; }
[ -e vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc/libminiupnpc.a ] || \
        PATH=".:C:\Users\nimbus\bin;C:\ProgramData\scoop\apps\git\2.44.0\mingw64\bin;C:\ProgramData\scoop\apps\git\2.44.0\usr\local\bin;C:\ProgramD
ata\scoop\apps\git\2.44.0\usr\bin;C:\ProgramData\scoop\apps\git\2.44.0\usr\bin;C:\ProgramData\scoop\apps\git\2.44.0\mingw64\bin;C:\ProgramData\scoo
p\apps\git\2.44.0\usr\bin;C:\Users\nimbus\bin;C:\ProgramData\scoop\apps\mingw-nuwen\current\bin;C:\ProgramData\scoop\apps\gcc\current\bin;C:\Progra
mData\scoop\apps\python\current\Scripts;C:\ProgramData\scoop\apps\python\current;C:\ProgramData\scoop\shims;C:\Windows\system32;C:\Windows;C:\Windo
ws\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\Program Files\WireGuard;C:\Windows\system32\config\syste
mprofile\AppData\Local\Microsoft\WindowsApps;C:\Users\nimbus\AppData\Local\Microsoft\WindowsApps;C:\ProgramData\scoop\apps\git\2.44.0\usr\bin\vendo
r_perl;C:\ProgramData\scoop\apps\git\2.44.0\usr\bin\core_perl" "C:/ProgramData/scoop/apps/mingw-nuwen/current/bin/make.exe" -C vendor/nim-nat-trave
rsal/vendor/miniupnp/miniupnpc -f Makefile.mingw CC=gcc libminiupnpc.a
make[1]: Entering directory 'D:/beacon-node-holesky-unstable/repo/vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc'
process_begin: CreateProcess(NULL, git rev-parse --short HEAD, ...) failed.
Makefile.mingw:56: pipe: No error
gcc -Os -Wall -W -Wstrict-prototypes -DNDEBUG -D_WIN32_WINNT=0x501 -Iinclude -I. -o wingenminiupnpcstrings.exe wingenminiupnpcstrings.c
gcc: fatal error: cannot execute 'cc1': CreateProcess: No such file or directory
compilation terminated.
make[1]: *** [Makefile.mingw:121: wingenminiupnpcstrings.exe] Error 1
make[1]: Leaving directory 'D:/beacon-node-holesky-unstable/repo/vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc'
make: *** [vendor/nimbus-build-system/makefiles/targets.mk:134: libminiupnpc.a] Error 2

And here's the same command with PATH=".:${PATH}" removed from makefiles/targets.mk:

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-unstable/repo (unstable)
$ rm -f vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc/{libminiupnpc.a,wingenminiupnpcstrings.exe}

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-unstable/repo (unstable)
$ make libminiupnpc.a V=3
which gcc &>/dev/null || { echo "C compiler (gcc) not installed. Aborting."; exit 1; }
[ -e vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc/libminiupnpc.a ] || \
        "C:/ProgramData/scoop/apps/mingw-nuwen/current/bin/make.exe" -C vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc -f Makefile.mingw CC=gcc
 libminiupnpc.a
make[1]: Entering directory 'D:/beacon-node-holesky-unstable/repo/vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc'
gcc -Os -Wall -W -Wstrict-prototypes -DNDEBUG -D_WIN32_WINNT=0x501 -Iinclude -I. -o wingenminiupnpcstrings.exe wingenminiupnpcstrings.c
.\wingenminiupnpcstrings.exe miniupnpcstrings.h.in miniupnpcstrings.h rc_version.h
Windows 6.2 Build 9200
MiniUPnPc version 2.2.6
27 lines written to miniupnpcstrings.h.
rc_version.h written
gcc -Os -Wall -W -Wstrict-prototypes -DNDEBUG -D_WIN32_WINNT=0x501 -Iinclude -I. -DMINIUPNP_STATICLIB -c -o miniwget.o src/miniwget.c
gcc -Os -Wall -W -Wstrict-prototypes -DNDEBUG -D_WIN32_WINNT=0x501 -Iinclude -I. -DMINIUPNP_STATICLIB -c -o minisoap.o src/minisoap.c
ar cr libminiupnpc.a miniwget.o minixml.o igd_desc_parse.o minisoap.o minissdpc.o miniupnpc.o upnpreplyparse.o upnpcommands.o upnperrors.o connecth
ostport.o portlistingparse.o receivedata.o upnpdev.o addr_is_reserved.o
make[1]: Leaving directory 'D:/beacon-node-holesky-unstable/repo/vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc'

Copy link
Member Author

Choose a reason for hiding this comment

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

The key part is this:

gcc -Os -Wall -W -Wstrict-prototypes -DNDEBUG -D_WIN32_WINNT=0x501 -Iinclude -I. -o wingenminiupnpcstrings.exe wingenminiupnpcstrings.c           
gcc: fatal error: cannot execute 'cc1': CreateProcess: No such file or directory

For some reason it fails when PATH is modified to include . at the beginning.

Copy link
Member Author

Choose a reason for hiding this comment

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

If I replace this line:

PATH=".:${PATH}" "$(MAKE)" -C vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc -f Makefile.mingw CC=$(CC) $@ $(HANDLE_OUTPUT)

With:

PATH=".:${PATH}" gcc -print-prog-name=cc1

I get:

/usr/bin/sh: line 2: gcc: command not found

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, I think I see what it is, It's most probably that the actual divider in PATH on windows is ;:

[ -e vendor/nim-nat-traversal/vendor/miniupnp/miniupnpc/libminiupnpc.a ] || \                                                                      
        PATH=".:C:\Users\nimbus\bin;C:\ProgramData\scoop\apps\git\2.44.0\mingw64\bin;C:\ProgramData\scoop\apps\git\2.44.0\usr\local\bin;C:\ProgramD
ata\scoop\apps\git\2.44.0\usr\bin;C:\ProgramData\scoop\apps\git\2.44.0\usr\bin;C:\ProgramData\scoop\apps\git\2.44.0\mingw64\bin;C:\ProgramData\scoo
p\apps\git\2.44.0\usr\bin;C:\Users\nimbus\bin;C:\ProgramData\scoop\apps\mingw-nuwen\current\bin;C:\ProgramData\scoop\apps\gcc\current\bin;C:\Progra
mData\scoop\apps\python\current\Scripts;C:\ProgramData\scoop\apps\python\current;C:\ProgramData\scoop\shims;C:\Windows\system32;C:\Windows;C:\Windo
ws\System32\Wbem;C:\Windows\System32\WindowsPowerShell\v1.0;C:\Windows\System32\OpenSSH;C:\Program Files\WireGuard;C:\Windows\system32\config\syste
mprofile\AppData\Local\Microsoft\WindowsApps;C:\Users\nimbus\AppData\Local\Microsoft\WindowsApps;C:\ProgramData\scoop\apps\git\2.44.0\usr\bin\vendo
r_perl;C:\ProgramData\scoop\apps\git\2.44.0\usr\bin\core_perl" gcc -print-prog-name=cc1                                                           
/usr/bin/sh: line 2: gcc: command not found  

Copy link
Member Author

Choose a reason for hiding this comment

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

do we need the if windows at all?

Indeed a good question. Let me test it without the if.

Copy link
Member Author

Choose a reason for hiding this comment

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

Well, the main difference is that in the if the -f Makefile.mingw flag is used, but it works either way.

Linux Way

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-unstable/repo (unstable)                                                                         
$ time make libminiupnpc.a 
real    0m14.249s                                                                                                                                 
user    0m0.015s                                                                                                                                  
sys     0m0.015s                                                                                                                                  

Windows Way

nimbus@windows-01 MINGW64 /d/beacon-node-holesky-unstable/repo (unstable)                                                                         
$ time make libminiupnpc.a                                                                                                                        
dllwrap: WARNING: dllwrap is deprecated, use gcc -shared or ld -shared instead                                                                    
                                                                                                                                                  
                                                                                                                                                  
real    0m20.171s                                                                                                                                 
user    0m0.000s                                                                                                                                  
sys     0m0.000s  

It is a bit slower. It does seem to build more files than the linux target.

Copy link
Member Author

Choose a reason for hiding this comment

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

The fact that there's a separate Makefile.mingw suggests it should be used.

Copy link
Member Author

Choose a reason for hiding this comment

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

The miniupnp docs state:

To compile, simply run 'gmake' (could be 'make' on your system).
Under win32, to compile with MinGW, type "mingw32make.bat".
MS Visual C solution and project files are supplied in the msvc/ subdirectory.
The miniupnpc library is available as a static library or as a DLL :
define MINIUPNP_STATICLIB if you want to link against the static library.

And mingw32make.bat is just:

@mingw32-make -f Makefile.mingw %1
@if errorlevel 1 goto end
@if not exist upnpc-static.exe goto end
@strip upnpc-static.exe
@upx --best upnpc-static.exe
@strip upnpc-shared.exe
@upx --best upnpc-shared.exe
:end

So it does explicitly use Makefile.mingw. Not using is probably a way to introduce weird issues.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the PR to just fix the format of PATH setting. The smaller the change the better.

@arnetheduck
Copy link
Member

ah, here: #81

Also use correct `;` divider. Otherwise the build fails with:
```
nimbus@windows-01 MINGW64 /d/beacon-node-holesky-libp2p/repo (nim-libp2p-auto-bump-unstable)
$ make libminiupnpc.a --debug
Reading makefiles...
Updating makefiles....
Updating goal targets....
 File 'libminiupnpc.a' does not exist.
   File 'sanity-checks' does not exist.
  Must remake target 'sanity-checks'.
  Successfully remade target file 'sanity-checks'.
Must remake target 'libminiupnpc.a'.
process_begin: CreateProcess(NULL, git rev-parse --short HEAD, ...) failed.
Makefile.mingw:56: pipe: No error
gcc: fatal error: cannot execute 'cc1': CreateProcess: No such file or directory
compilation terminated.
make[1]: *** [Makefile.mingw:121: wingenminiupnpcstrings.exe] Error 1
make: *** [vendor/nimbus-build-system/makefiles/targets.mk:134: libminiupnpc.a] Error 2
```
Resolves:
status-im/nimbus-eth2#5507

I still don't get why this `PATH` modification is even necessary, it works without it.

Signed-off-by: Jakub Sokołowski <[email protected]>
@jakubgs jakubgs force-pushed the fix-miniupnpc-windows-build branch from 6accb2a to 998ab9f Compare March 27, 2024 07:52
@jakubgs jakubgs changed the title fix miniupnpc build on Windows by not changing PATH fix miniupnpc build on Windows by not escaping PATH Mar 27, 2024
@jakubgs jakubgs merged commit d050836 into master Mar 27, 2024
15 checks passed
@jakubgs jakubgs deleted the fix-miniupnpc-windows-build branch March 27, 2024 08:44
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