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

Port important files to fish #82

Closed
wants to merge 1 commit into from
Closed

Conversation

grhkm21
Copy link

@grhkm21 grhkm21 commented Sep 14, 2023

I have ported

  • activate.sh (important)
  • coverage.sh
  • build_inplace.sh (important)
  • build_variables.sh
  • build_dependencies_unix.sh (very important)

to fish, and also added a README for it (at run_me.sh and run_me.fish). activate.fish doesn't behave the same as activate.sh but I don't think anyone cares.

Also has a run_me.[fi]sh which serves as a README for `bin`, I guess.
I might port the other scripts if I find them useful.
@oscarbenjamin
Copy link
Collaborator

I don't want to duplicate all of the shell scripts. Why not just improve the existing scripts if there are changes to be made?

Alternatively if the shell scripts are not compatible with fish then perhaps migrate them to Python instead.

Longer term it would be better to move away from most of these scripts but the way to do that is first to change build system #52. The reason that I haven't done much to improve these scripts is just because they should really be commands that call into a build system i.e. most of this code should really be in meson files.

@grhkm21
Copy link
Author

grhkm21 commented Sep 14, 2023

Ah, sorry for that. Yes, my main intent was just to make it compatible with both bash and fish, or more precisely just pushing the translations I have made.

Translating to Python would work, but I will have to look into it, I am not extremely familiar with it. As for new build systems, I will leave it for others.

I will close this for now then :)

@grhkm21 grhkm21 closed this Sep 14, 2023
@oscarbenjamin
Copy link
Collaborator

Adding activate.fish is fine but for example we don't want a duplicate of build_variables.sh because that defines the versions that are used for the build which should only be defined in one place. Also build_dependencies_unix.sh is the script that is tested and used in CI and so we don't want to duplicate that and have to maintain two copies (with one not being tested).

Apart from activate.fish can you not just do bash bin/build_dependencies.sh to run the build under fish?

Python scripts would be better than a shell scripts though because it is better to avoid using shell scripts for anything more than just putting together a few commands. Anyone looking to build python-flint needs to have Python installed anyway so it is not an issue in that sense.

We would not have had this bug if Python was being used instead of shell scripts: #57

@grhkm21
Copy link
Author

grhkm21 commented Sep 15, 2023

Oh yes, you are correct 🤦 For a Python script, would something like this script I wrote suffice? Not sure if that is what you mean for a Python installation script? I am inexperienced with these things but I can try :D

Of course replace the print statements with logging.log or something

@oscarbenjamin
Copy link
Collaborator

For a Python script, would something like this script I wrote suffice

Yes, that's the sort of thing I was thinking.

I think that print statements are fine but I would use argparse to add command line options for all things.

It should support all of the things that are currently supported by bin/build_dependencies.sh and should then be used instead of bin/build_dependencies.sh in all relevant places:

$ git grep build_dependencies
bin/build_mingw64_wheel.sh:#bin/build_dependencies_unix.sh
bin/build_wheel.sh:# build_dependencies_unix.sh (which should be run first).
bin/cibw.sh:# bin/build_dependencies_unix.sh places headers and shared libraries under .local
bin/cibw_before_all_linux.sh:bin/build_dependencies_unix.sh\
bin/cibw_before_all_macosx_arm64.sh:bin/build_dependencies_unix.sh\
bin/cibw_before_all_macosx_x86_64.sh:bin/build_dependencies_unix.sh\
bin/cibw_before_all_windows.sh:bin/build_dependencies_unix.sh --use-gmp-github-mirror
doc/source/setup.rst:    bin/build_dependencies_unix.sh
doc/source/setup.rst:the ``build_dependencies_unix.sh`` script to work. After running the script,

The script also needs to be able to run on Linux, OSX and Windows in the msys2 shell.

If the script has non-stdlib dependencies (e.g. requests) then their should be instructions for installing those somewhere or a requirements.txt if there are many.

@grhkm21
Copy link
Author

grhkm21 commented Sep 15, 2023

Okay! I will work on it.

@oscarbenjamin
Copy link
Collaborator

The build_variables.sh file could also be removed and the versions could be set at the top of the new Python script.

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