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

Little fix for cygwin&msys2 environment #3569

Closed
wants to merge 1 commit into from
Closed

Little fix for cygwin&msys2 environment #3569

wants to merge 1 commit into from

Conversation

proukornew
Copy link
Contributor

No description provided.

gvreuls referenced this pull request Jun 19, 2021
The Cygwin environment has two g++ compilers, each with a different problem
for compiling  Stockfish at the moment:

(a) g++.exe : full posix build compiler, linked to cygwin dll.

    => This one has a problem embedding the net.

(b) x86_64-w64-mingw32-g++.exe : native Windows build compiler.

    => This one manages to embed the net, but has a problem related to libgcov
       when we use the profile-build target of Stockfish.

This patch solves the problem for compiler (b), so that our recommended command line
if you want to build an optimized version of Stockfish on Cygwin becomes something
like the following (you can change the ARCH value to whatever you want, but note
the COMP and CXX variables pointing at the right compiler):

```
   make -j profile-build ARCH=x86-64-modern COMP=mingw CXX=x86_64-w64-mingw32-c++.exe
```

closes #3463

No functional change
@vondele vondele added the to be merged Will be merged shortly label Jun 19, 2021
@vondele
Copy link
Member

vondele commented Jun 19, 2021

I'll merge this one shortly

@vondele vondele closed this in 0171b50 Jun 19, 2021
@vondele
Copy link
Member

vondele commented Jun 24, 2021

seems like this again broke something now compiling under linux within fishtest. Might need another revert official-stockfish/fishtest#952

@vondele vondele reopened this Jun 24, 2021
@gvreuls
Copy link
Contributor

gvreuls commented Jun 24, 2021

It's unfortunate this silently breaks fishtest because the solution was tested after the revert of the original patch and seemed to work perfectly.

I think this isn't our bug per se but something going wrong with what the ffishtest worker assumes is its current working directory, either because of a bug in games.py (where some directory juggling is going on) or perhaps even in the python interpreter itself (Python 3.9.5 on my workers).

@proukornew
Copy link
Contributor Author

proukornew commented Jun 24, 2021

subprocess.check_call(
            MAKE_CMD + ARCH + " -j {}".format(concurrency) + " profile-build",
            shell=True,
            env=dict(os.environ, CXXFLAGS="-DNNUE_EMBEDDING_OFF"),
        )
        try:  # try/pass needed for backwards compatibility with older stockfish, where 'make strip' fails under mingw.
            subprocess.check_call(
                MAKE_CMD + ARCH + " -j {}".format(concurrency) + " strip", shell=True
            )
        except:
            pass
        shutil.move("stockfish" + EXE_SUFFIX, destination)

Look consistent and no directory juggling

@gvreuls
Copy link
Contributor

gvreuls commented Jun 24, 2021

   tmp_dir = tempfile.mkdtemp(dir=worker_dir)

    try:
        os.chdir(tmp_dir)
        with open("sf.gz", "wb+") as f:
            f.write(
                requests.get(
                    github_api(repo_url) + "/zipball/" + sha, timeout=HTTP_TIMEOUT
                ).content
            )
        zip_file = ZipFile("sf.gz")
        zip_file.extractall()
        zip_file.close()

        os.chdir(glob.glob(os.path.join(tmp_dir, "*/src/"))[0])

Looks like directory juggling to me, and I don't know if os.chdir() changes the python working directory too, I'm not a python specialist.

@gvreuls
Copy link
Contributor

gvreuls commented Jun 24, 2021

The error with the fishtest worker only occurs if the ccache compiler cache is installed and disabling the cache for the worker fixes the problem, so this is due to ccache incorrectly assuming that the instrumented object files created during the make profile-build process are cacheable and reusable in later builds.

The patch is fine but exposes a ccache bug that's easily mitigable by disabling the cache for the worker (by setting CCACHE_DISABLE="1" in the worker's environment).

@vondele vondele closed this Jun 24, 2021
MichaelB7 pushed a commit to MichaelB7/Stockfish that referenced this pull request Jul 6, 2021
The Cygwin environment has two g++ compilers, each with a different problem
for compiling  Stockfish at the moment:

(a) g++.exe : full posix build compiler, linked to cygwin dll.

    => This one has a problem embedding the net.

(b) x86_64-w64-mingw32-g++.exe : native Windows build compiler.

    => This one manages to embed the net, but has a problem related to libgcov
       when we use the profile-build target of Stockfish.

This patch solves the problem for compiler (b), so that our recommended command line
if you want to build an optimized version of Stockfish on Cygwin becomes something
like the following (you can change the ARCH value to whatever you want, but note
the COMP and CXX variables pointing at the right compiler):

```
   make -j profile-build ARCH=x86-64-modern COMP=mingw CXX=x86_64-w64-mingw32-c++.exe
```

closes official-stockfish#3569

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.

3 participants