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

Account for distutils being deprecated and python3.10 becoming default on Ubuntu 22.04 #348

Closed
wants to merge 19 commits into from

Conversation

Patol75
Copy link
Contributor

@Patol75 Patol75 commented Jan 17, 2022

Closes #346

I am opening this PR as a draft; it will be needed in the future given changes in the Python distribution. Please feel free to contribute, as it is likely I will succeed in figuring everything out.

I have tried to identify instances where distutils is used and replace them with equivalent logic from either setuptools or sysconfig.

Something of potential interest is setuptools indicating that the setup.py approach is deprecated. Instead, pyproject.toml and setup.cfg files should be used, in combination with the build and pip packages. I have tried to build the fluidity package this way.


Judging from Actions, Focal is successful, but Bionic does not find a valid version of Python.

@Patol75 Patol75 requested a review from stephankramer January 17, 2022 06:23
@Patol75 Patol75 added help wanted WIP work-in-progress PRs that are known to be not ready for merge labels Jan 17, 2022
@Patol75
Copy link
Contributor Author

Patol75 commented Jan 19, 2022

I think this is starting to head in the right direction. I am giving it a CI test to see if the new python_build behaves.

@stephankramer

  • Could you let me know what the aclocal.m4 files are and what they do? I have modified them to mirror the changes in the configure files, but it is not clear to me what this actually does.
  • To account for the deprecated setup.py approach and the new recommended build + pip strategy, I have removed default and fltools from the install target in Makefile.in. The main reason is that pip should not be run as root (I still do it in the clean target, but I guess this is more acceptable). Considering default and fltools do not require root privileges, but install does, I am not sure if it made sense to have them in. Additionally, an improvement from the new pip logic is that PYTHONPATH does not need to be set anymore to access fluidity's python modules.
  • Some C++ files in tools were apparently missing the unistd.h header inclusion. I am not quite sure how chdir was executed before.

@gnikit
Copy link
Member

gnikit commented Jan 20, 2022

@Patol75 you might want to have a look at https://github.com/nektos/act . I use it for local Actions testing. Maybe you can use it with only a couple failing tests, on a single docker container and get some info on the errors.

Also, I am not sure if this is relevant but HAVE_NUMPY if true, is never actually #defined in confdefs.h like the rest of our preprocessor macros, so that might lead to some cryptic failures.

@Patol75
Copy link
Contributor Author

Patol75 commented Jan 21, 2022

Thanks for the suggestion. I tried, but I did not manage to get a successful build, unfortunately. I thought that fluidity being in the sudo group was the reason that Python could not find installed libraries, but apparently not.

Locally, if I run ./configure --enable-2d-adaptivity CPPFLAGS=-I/usr/local/include && make makefiles && sudo make clean && make -j && make -j fltools, followed by tools/testharness.py -f top_hat_cv.xml, I get Passes. Python libraries are properly installed at /home/thomas/.local/lib/python3.8/site-packages, and that is where they are imported from.

@Patol75
Copy link
Contributor Author

Patol75 commented Jan 25, 2022

I am closing this PR as I will likely open a new one based on the deprecate_distutils_2 branch.

@Patol75 Patol75 closed this Jan 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted WIP work-in-progress PRs that are known to be not ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replace distutils by setuptools
2 participants