-
Notifications
You must be signed in to change notification settings - Fork 177
Fix install script to use Netgen fork #3147
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
Conversation
Signed-off-by: Umberto Zerbinati <umberto.zerbinati@maths.ox.ac.uk>
| run_pip(["install", "ngsolve", "--pre"]) | ||
| log.info("Installing scikit-build.") | ||
| run_pip(["install", "-U", "scikit-build"]) | ||
| log.info("Installing Netgen, it may take an hour or more to build!") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why ? Because we are building Netgen from source ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think more because building Netgen for an hour is going to significantly slow down CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it's going to properly irritate users. If this is as big as this and you need a branch, I think you need to provide wheels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do mean you need a branch? I'm not familiar with wheels I'll start looking into it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line 1422 appears to indicate that you're doing this because you need your fork of netgen rather than the version on PyPI.
The version on PyPI will be compiled python packages, which are called wheels. If it takes this long to build netgen then you need to compile your own Python packages and use them. Netgen is already set up as a Python package, so building the wheels should be easy (see https://packaging.python.org/en/latest/tutorials/packaging-projects/#generating-distribution-archives)
You can either put these packages on PyPI under an alternative name, or you can make a release of your project on GitHub and grab the wheels from there. You can look at how we deal with vtk in the install script as an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll start looking into this immediately.
Signed-off-by: Umberto Zerbinati <umberto.zerbinati@maths.ox.ac.uk>
name: "Bug fix or new feature"
description: ''
title: ''
labels: ''
assignees: ''
Description
Please include a summary of the changes introduced by this PR.
Additionally include any relevant motivation and context. List any
dependencies that are required for this change, other PRs upon
which this may depend, and associated issues.
Associated Pull Requests:
Fixes Issues:
If issues are fixed by this PR, prepend each of them with the word
"fixes", so they are automatically closed when this PR is merged. For
example "fixes #123, fixes #456".
Checklist for author:
make lintin thefiredrakesource directory).make linkcheck; make html; make latexpdfinfiredrake/docsdirectory)pytest testsin thefiredrakesource directory) (useful, but not essential if you don't have suitable hardware).Checklist for reviewer:
Feel free to add reviewers if you know there is someone who is already aware of this work.
Please open this PR initially as a draft and mark as ready for review once CI tests are passing.