search

official-pikafish / Pikafish

UCI xiangqi engine
http://pikafish.org
GNU General Public License v3.0
867 stars 162 forks source link

Add M1 mac build #68

Closed jeffli678 closed 7 months ago

jeffli678 commented 7 months ago

This PR does two things:

  1. It updates the action to use upload-artifacts@v4, which can properly archive the build artifacts.
  2. It adds a new job that builds Pikafish on the newly available M1 runners. I first tried to integrate that into the matrix, but the exclude rules soon becomes very inflated, and I figured it would be better to create a separate job for the M1 build
PikaCat-OuO commented 7 months ago

I think since we decide to use v4 for uploading, then we should remove "Copy binary to folder" and directly upload the generated binary. Apart from that, the extra file (Wiki, AUTHORS.....) for Android build is not complete, they should be added.

PikaCat-OuO commented 7 months ago

I'm still wondering if it should be better to include the nnue file also in the generated artifacts.

PikaCat-OuO commented 7 months ago

Maybe also add ur name to the AUTHORS file.

jeffli678 commented 7 months ago

I have:

  1. Added myself into the AUTHORS list
  2. Fixed the android build missing certain files
  3. Included pikafish.nnue in the build artifact. For this, I did not remove the "copy to folder" step because that will cause the binary and nnue file be put in a folder called "src", which is a bit weird. You can see the latest CI https://github.com/jeffli678/Pikafish/actions/runs/7809297290 to see if the folder structure looks good to you
  4. I noticed the current build artifact does not include the README for the nnue file, i.e., https://github.com/official-pikafish/Networks/blob/master/README.md. Do you think we should also somehow include it?
PikaCat-OuO commented 7 months ago

For this, I did not remove the "copy to folder" step because that will cause the binary and nnue file to be put in a folder called "src", which is a bit weird.

I think the structure has to make the binary directly available on the same folder as those files like "README.md". So that's why I suggest removing the "copy to folder" and possibly adding "mv $EXE ../" in "compile xxx build".

I'm still wondering if it should be better to include the nnue file also in the generated artifacts.

Adding the nnue inside the build seems too large. I'm just "wondering" at that time. Maybe it's better to leave the action to produce artifacts w/o it then.

That's pretty much ready for merging.

jeffli678 commented 7 months ago

For this, I did not remove the "copy to folder" step because that will cause the binary and nnue file to be put in a folder called "src", which is a bit weird.

I think the structure has to make the binary directly available on the same folder as those files like "README.md". So that's why I suggest removing the "copy to folder" and possibly adding "mv $EXE ../" in "compile xxx build".

I'm still wondering if it should be better to include the nnue file also in the generated artifacts.

Adding the nnue inside the build seems too large. I'm just "wondering" at that time. Maybe it's better to leave the action to produce artifacts w/o it then.

That's pretty much ready for merging.

I have:

  1. Removed the "copy to folder" step and added a "cp $EXE ../" in the compile step
  2. I have removed the nnue file from the build archive

I squashed the commit and prepared it for the merge

PikaCat-OuO commented 7 months ago

Looks good to me.