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

Recursive hell of vars #3859

Closed
wants to merge 3 commits into from

Conversation

proukornew
Copy link
Contributor

@proukornew proukornew commented Dec 16, 2021

Variables had different behavior via recursive dependency hell.
If there is no environment variable -> sometimes variable treated as local with correct behavior.
If there is environment variable -> variable treated as global with multiple extending.

E.g.
CXXFLAGS="-march=native -DNNUE_EMBEDDING_OFF" make COMP=mingw ARCH=x86-64-modern -j3 profile-build CXX=x86_64-w64-mingw32-g++.exe

Was:
x86_64-w64-mingw32-g++.exe -march=native -DNNUE_EMBEDDING_OFF -Wall -Wcast-qual -fno-exceptions -std=c++17 -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -Wall -Wcast-qual -fno-exceptions -std=c++17 -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -Wall -Wcast-qual -fno-exceptions -std=c++17 -fprofile-generate=profdir -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -c -o bitboard.o bitboard.cpp

Now:
x86_64-w64-mingw32-c++ -Wall -Wcast-qual -fno-exceptions -std=c++17 -fprofile-generate=profdir -march=native -DNNUE_EMBEDDING_OFF -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -c -o bitboard.o bitboard.cpp
...

Variables had different behavior via recursive dependency hell.
If there is no environment variable -> sometimes variable treated as local with correct behavior or no.
If there is environment variable      -> variable treated as global with multiple extending.

E.g. 
CXXFLAGS="-march=native -DNNUE_EMBEDDING_OFF" make COMP=mingw ARCH=x86-64-modern -j3 profile-build CXX=x86_64-w64-mingw32-g++.exe
...
x86_64-w64-mingw32-g++.exe -march=native -DNNUE_EMBEDDING_OFF -Wall -Wcast-qual -fno-exceptions -std=c++17  -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -Wall -Wcast-qual -fno-exceptions -std=c++17  -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2 -Wall -Wcast-qual -fno-exceptions -std=c++17 -fprofile-generate=profdir -pedantic -Wextra -Wshadow -DNDEBUG -O3 -DIS_64BIT -msse -msse3 -mpopcnt -DUSE_POPCNT -DUSE_SSE41 -msse4.1 -DUSE_SSSE3 -mssse3 -DUSE_SSE2 -msse2   -c -o bitboard.o bitboard.cpp

Now:
CMDCXXFLAGS="-march=native -DNNUE_EMBEDDING_OFF" make COMP=mingw ARCH=x86-64-modern -j3 profile-build CXX=x86_64-w64-mingw32-g++.exe
@proukornew proukornew marked this pull request as ready for review December 16, 2021 22:45
@dubslow
Copy link
Contributor

dubslow commented May 4, 2022

Is there any particular reason that this patch hasn't been merged?

@vondele
Copy link
Member

vondele commented May 4, 2022

I'm not convinced that introducing yet another variable is a good solution for the problem. Additionally, if this is introduced without documentation it doesn't help. I believe the proper solution is to make CXXFLAGS behave as expected.

@proukornew
Copy link
Contributor Author

@vondele , check this.

@proukornew
Copy link
Contributor Author

ping @vondele

@vondele vondele added the to be merged Will be merged shortly label May 29, 2022
@vondele vondele closed this in 6ede1be May 29, 2022
dav1312 pushed a commit to dav1312/Stockfish that referenced this pull request Oct 21, 2022
removes duplication on the commandline for example in a profile-build

closes official-stockfish#3859

No functional change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
to be merged Will be merged shortly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants