search

gpilab / framework

The GPI framework provides the canvas for graphically assembling algorithms.
Other
20 stars 7 forks source link

Minor updates to gpi_make #27

Closed borupdaniel closed 5 years ago

borupdaniel commented 5 years ago

I haven't tested these but since it's only adding a flag for gpi_make, and will otherwise behave the same, I'd be fine merging this and calling it v1.1.3 – thoughts?

borupdaniel commented 5 years ago

@aganders3 I think the latest commit sets the order of the include and library paths to have the conda libraries first, as we discussed last week.

Can you take a look at this and let me know if you think it's ready to merge? I'd like to get back to testing the core nodes so we can get them up on conda-forge this week if possible!

borupdaniel commented 5 years ago

Note I did leave those after anything set in the gpirc file; seems sensible to leave that as the one place a user can override what we've specified if needed.

aganders3 commented 5 years ago

Thanks - seems good to me. Have you tested locally if this gives the behavior we want? I’m fine with leaving the user libs first as these changes are pretty much just for the binary distributions.

On Mon, Jul 22, 2019 at 11:37 Daniel Borup notifications@github.com wrote:

Note I did leave those after anything set in the gpirc file; seems sensible to leave that as the one place a user can override what we've specified if needed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gpilab/framework/pull/27?email_source=notifications&email_token=AAJMXVHFPGM4PTSNQYVQS23QAXO3HA5CNFSM4IEBJMLKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2QN5XY#issuecomment-513859295, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJMXVALH23KNL3PJWK64ZLQAXO3HANCNFSM4IEBJMLA .

borupdaniel commented 5 years ago

I tested it locally and it seems like setting both --ignore-system-libs and --ignore-gpirc led to the make not finding any node libraries. I think your implementation was blocking this in the recursive search_dir section so I changed that to look in what should be site-packages for a conda install if both flags are set. Seems to be working for me now.

aganders3 commented 5 years ago

This looks good and non-breaking to me if you want to merge for testing on conda-forge