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

cmake install fixes #2596

Merged
merged 39 commits into from
Nov 3, 2022
Merged

cmake install fixes #2596

merged 39 commits into from
Nov 3, 2022

Conversation

dschwoerer
Copy link
Contributor

@dschwoerer dschwoerer commented Oct 4, 2022

Installs boutcore boutpp

Resolves one of the points of: #2321

We guess the location, but it can be changed with
-DCMAKE_INSTALL_PYTHON_SITEARCH
running `make all install` in manual folder no builds and installs the
html manual
Skip unit test if gtest is not available
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @dschwoerer !

@dschwoerer
Copy link
Contributor Author

rpath is broken, so still WIP

dschwoerer and others added 21 commits October 17, 2022 14:24
requires a wrapper around libboutcore
It still ignore the python version, and uses what ever python version
cmake can find.
`py.310 -m build` and `py.311 -m build` give the same results.
`-C` flags can be used to tell cmake to use a given python version.
removesuffix is only python3.9+
otherwise the error is lost as only the last exit code is checked
The python zip implementation doesn't support them, so we end up with
broken symlinks and thus unloadable libaries.
@dschwoerer
Copy link
Contributor Author

Should be working now 👍

Unlike before the cython library is not directly imported, but a wrapper imports everything (except symbols starting with _)

@dschwoerer
Copy link
Contributor Author

dschwoerer commented Oct 19, 2022

Nice to have:

pip install boutpp
module use $(python -c 'import site; print(site.getusersitepackages())')/boutpp/share/module
module load bout++/python

Should this be split into several PRs?
Renaming as part of the PR was potentially a bad idea, I worry that rebasing will lead to many conflicts 🙈

This was referenced Oct 19, 2022
Copy link
Member

@ZedThree ZedThree left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a custom backend is very clever and really nice! I have a few suggestions, and it could do with some docstrings.

Could pyproject.toml have the project metadata too, or does the backend handle all that?

I suspect this will need conflicts fixing after the boutpp numpy PR goes in.

Also, the black workflow is broken: https://github.com/boutproject/BOUT-dev/actions/runs/3377858435/jobs/5607304385#step:5:39
So that will need double-checking

CMakeLists.txt Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
@ZedThree
Copy link
Member

ZedThree commented Nov 2, 2022

Oh dear, #2589 and this PR both renamed a bunch of files. I had a quick go, but @dschwoerer you're probably going to have to deal with this, I'm afraid!

* use module-level caching
* Add docs on what to update on release
* use re.sub for substitution

Thanks @ZedThree for the comments
* Use more appropriate python libraries (mkdir, run2)
* Add doc-strings
* Remove commented out code

Thanks @ZedThree for the comments
Copy link
Contributor Author

@dschwoerer dschwoerer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback 👍

CMakeLists.txt Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
tools/pylib/_boutpp_build/backend.py Outdated Show resolved Hide resolved
@dschwoerer
Copy link
Contributor Author

Using a custom backend is very clever and really nice! I have a few suggestions, and it could do with some docstrings.

Could pyproject.toml have the project metadata too, or does the backend handle all that?

It could, but then it needs to be parsed + reformatted and printed. Might be easier to add it directly. But more metadata is one of the missing things.

I suspect this will need conflicts fixing after the boutpp numpy PR goes in.

Yes. Should be fixed now.

Also, the black workflow is broken: https://github.com/boutproject/BOUT-dev/actions/runs/3377858435/jobs/5607304385#step:5:39 So that will need double-checking

black seems to be fixed. The ! did the job :-)

ZedThree
ZedThree previously approved these changes Nov 3, 2022
@ZedThree
Copy link
Member

ZedThree commented Nov 3, 2022

Some tests failing like:

  File "/home/runner/work/BOUT-dev/BOUT-dev/build/tests/MMS/bracket/runtest", line 17, in <module>
    import boutpp as bc
ValueError: source code string cannot contain null bytes

https://github.com/boutproject/BOUT-dev/actions/runs/3385119775/jobs/5622897973#step:9:823

@ZedThree ZedThree merged commit 4acdfdc into next Nov 3, 2022
@ZedThree ZedThree deleted the cmake-install branch November 3, 2022 14:22
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.

None yet

2 participants