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

Export and clean up net downloading script #5563

Closed
wants to merge 2 commits into from

Conversation

MinetaS
Copy link
Contributor

@MinetaS MinetaS commented Sep 2, 2024

This patch extracts the net downloading script in Makefile into an external script file. Also the script is moderately rewritten for improved readability and speed.

  • Use wget preferentially over curl, as curl is known to have slight overhead.
  • Use command instead of hash to check if command exists. Reportedly, hash always returns zero in some POSIX shells even when the command fails.
  • Command existence checks (wget/curl, sha256sum) are performed only once at the beginning.
  • Each of common patterns is encapsulated in a function (get_nnue_filename, validate_network).
  • Print out error/warning messages to stderr.

No functional change

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 2, 2024

This PR is a part of #5543 and submitted independently thanks to Disservin's advice.

20 fishtest workers ran tasks successfully:
https://tests.stockfishchess.org/tests/view/66d57a5f9de3e7f9b33d147a

@Disservin
Copy link
Member

Can you check if this patch fixes the CI issues, and just include it here?

diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index 8d209a4f..1e1a1280 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -342,8 +342,8 @@ jobs:
       - name: Test riscv64 build
         if: matrix.config.run_riscv64_tests
         run: |
-          echo "export LDFLAGS='-static' && make clean && make -j4 ARCH=riscv64 build" > script.sh
-          docker run --rm --platform ${{ matrix.config.platform }} -v ${{ github.workspace }}/src:/app sf_builder
+          echo "cd src && export LDFLAGS='-static' && make clean && make -j4 ARCH=riscv64 build" > script.sh
+          docker run --rm --platform ${{ matrix.config.platform }} -v ${{ github.workspace }}:/app sf_builder
           ../tests/signature.sh $benchref

       # ppc64 tests
@@ -351,8 +351,8 @@ jobs:
       - name: Test ppc64 build
         if: matrix.config.run_ppc64_tests
         run: |
-          echo "export LDFLAGS='-static' && make clean && make -j4 ARCH=ppc-64 build" > script.sh
-          docker run --rm --platform ${{ matrix.config.platform }} -v ${{ github.workspace }}/src:/app sf_builder
+          echo "cd src && export LDFLAGS='-static' && make clean && make -j4 ARCH=ppc-64 build" > script.sh
+          docker run --rm --platform ${{ matrix.config.platform }} -v ${{ github.workspace }}:/app sf_builder
           ../tests/signature.sh $benchref

       # Other tests

related to #5564

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 3, 2024

@Disservin It didn't work. Is workflow cwd /src?

sh: can't open 'script.sh': No such file or directory

@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 3, 2024

Changed Dockerfile to run sh src/script.sh and it looks good now.

@Disservin
Copy link
Member

Ah yes that too, thank.s

MinetaS and others added 2 commits September 3, 2024 21:00
This patch extracts the net downloading script in Makefile into an
external script file. Also the script is moderately rewritten for
improved readability and speed.

* Use wget preferentially over curl, as curl is known to have
  slight overhead.
* Use command instead of hash to check if command exists. Reportedly,
  hash always returns zero in some POSIX shells even when the command
  fails.
* Command existence checks (wget/curl, sha256sum) are performed only
  once at the beginning.
* Each of common patterns is encapsulated in a function
  (get_nnue_filename, validate_network).
* Print out error/warning messages to stderr.

No functional change
@Disservin Disservin added the to be merged Will be merged shortly label Sep 9, 2024
@Disservin Disservin closed this in 1b310cc Sep 9, 2024
@MinetaS
Copy link
Contributor Author

MinetaS commented Sep 9, 2024

Note here:

I read some issues today and one of them is about wget failing to connect on IPv6 (#4978). Although it's not even remotely close to Stockfish issue, if the default behavior of the command poses errors, I believe the logic has better be reverted back (detect and use curl primarily).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants