-
Notifications
You must be signed in to change notification settings - Fork 55
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
Build package updates #643
Build package updates #643
Conversation
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.
Looks good for the most part. I left a few minor questions and comments that should be easy to work through quickly. Thanks!
@@ -112,7 +112,7 @@ def build_extensions(self): | |||
sources=[ | |||
"troute/network/reservoirs/rfc/rfc.{}".format(ext), | |||
], | |||
include_dirs=[np.get_include()], | |||
include_dirs=[np.get_include(), "troute/network/"], |
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.
Is this an issue if you try to install from a different directory than the dir that has setup.py
?
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'm not sure about that, haha.
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.
Is this supposed to be in this PR? Seems unrelated. Seems related to #616.
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 was based on that branch, that is a shared commit 592afa7 so I can rebase this PR to clear it out if needed.
@hellkite500 is this PR still being worked on or is it ready for review/merge? |
@shorvath-noaa It should be good as is. Might need to do some testing based on @aaraney s comment and make some adjustments, but that can be done separately. |
@hellkite500 I got this to compile ok, but when I run t-route with RFC's on it still fails, so I'm not sure this fixes Issue 620. This is the error I get:
|
@shorvath-noaa This fixes the build/packaging -- what you are seeing is a runtime error that is a separate issue, I think. |
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.
Verified this installed correctly with python3.8
on Debian slim bullseye.
Some build/package updates