[SCons] Change prefix default if conda is detected by ischoegl · Pull Request #1191 · Cantera/cantera

ischoegl · 2022-02-08T21:49:31Z

Changes proposed in this pull request

The default installation prefix is changed only when:

prefix is not set AND
python_prefix is not set AND
python_cmd is not set AND
$CONDA_PREFIX is set AND
the Python running SCons is located in $CONDA_PREFIX

Also: add conda include/library folders if conda is used (as long as extra_inc_dirs and extra_lib_dirs are not set.

If applicable, fill in the issue number this pull request is fixing

Closes Cantera/enhancements#76

If applicable, provide an example illustrating new features this pull request is introducing

[...]
postInstallMessage(["finish_install"], [])

Cantera has been successfully installed.

File locations:

  applications                C:\Users\<user>\miniconda3\envs\cantera-dev\Scripts
  library files               C:\Users\<user>\miniconda3\envs\cantera-dev\Library\lib
  C++ headers                 C:\Users\<user>\miniconda3\envs\cantera-dev\Library\include
  samples                     C:\Users\<user>\miniconda3\envs\cantera-dev\share\cantera\samples
  data files                  C:\Users\<user>\miniconda3\envs\cantera-dev\share\cantera\data
  Python package (cantera)    C:\Users\<user>\miniconda3\envs\cantera-dev\Lib\site-packages
  Python samples              C:\Users\<user>\miniconda3\envs\cantera-dev\Lib\site-packages\cantera\examples

scons: done building targets.

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

codecov · 2022-02-08T22:15:02Z

Codecov Report

Merging #1191 (58be58a) into main (0119484) will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1191      +/-   ##
==========================================
- Coverage   65.45%   65.41%   -0.04%     
==========================================
  Files         318      318              
  Lines       46106    46085      -21     
  Branches    19604    19604              
==========================================
- Hits        30177    30145      -32     
+ Misses      13433    13426       -7     
- Partials     2496     2514      +18

Impacted Files	Coverage Δ
include/cantera/kinetics/Arrhenius.h	`81.91% <0.00%> (-16.11%)`	⬇️
include/cantera/kinetics/ReactionRate.h	`82.92% <0.00%> (-4.26%)`	⬇️
src/kinetics/ReactionFactory.cpp	`90.04% <0.00%> (-2.03%)`	⬇️
include/cantera/kinetics/MultiRate.h	`83.92% <0.00%> (-1.79%)`	⬇️
include/cantera/kinetics/RxnRates.h	`88.23% <0.00%> (-1.31%)`	⬇️
src/base/AnyMap.cpp	`85.10% <0.00%> (-0.19%)`	⬇️
include/cantera/kinetics/Reaction.h	`100.00% <0.00%> (ø)`
include/cantera/thermo/ThermoPhase.h	`30.57% <0.00%> (ø)`
src/thermo/MixtureFugacityTP.cpp	`37.50% <0.00%> (+0.13%)`	⬆️
src/kinetics/Reaction.cpp	`80.89% <0.00%> (+0.41%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0119484...58be58a. Read the comment docs.

bryanwweber

Thanks for proposing something here @ischoegl! I have a high level question before reviewing the code in more detail. I wonder if it would be clearer to a user if we added conda as an allowed default value for some of the options? Or, if that's too intrusive, appended/overwrote some of the default options before the options are all saved to cantera.conf. This could be used to write things out into the cantera.conf file, which may prompt people to edit that file to restore old behavior if they want. I think that could also simplify some of the use_conda logic and checking lines from cantera.conf. Does that make sense? I'd be happy to throw together a small prototype to clarify.

SConstruct

ischoegl · 2022-02-09T21:32:21Z

@bryanwweber ... thank you for the feedback! I limited the conda-specific installation folders to Windows (share btw also exists there, which I am now using). I still need to test on Linux ...

bryanwweber

This is really shaping up @ischoegl I'm 👍 on the direction in general here. A few comments/questions below

SConstruct

ischoegl · 2022-02-10T18:11:58Z

🎉 ... this is for Linux with the following conda environment:

$ conda create -n cantera-dev -c conda-forge sundials=6.0 scons numpy ruamel.yaml cython boost-cpp fmt eigen yaml-cpp libgomp openblas pytest ipython

This is the result of scons build & scons install ... which detects all 'system' dependencies (no additional options needed)

postInstallMessage(["finish_install"], [])

Cantera has been successfully installed.

File locations:

  applications                /home/<user>/.conda/envs/cantera-dev/bin
  library files               /home/<user>/.conda/envs/cantera-dev/lib
  C++ headers                 /home/<user>/.conda/envs/cantera-dev/include
  samples                     /home/<user>/.conda/envs/cantera-dev/share/cantera/samples
  data files                  /home/<user>/.conda/envs/cantera-dev/share/cantera/data  
  Python package (cantera)    /home/<user>/.conda/envs/cantera-dev/lib/python3.10/site-packages
  Python samples              /home/<user>/.conda/envs/cantera-dev/lib/python3.10/site-packages/cantera/examples

Setup scripts to configure the environment for Cantera are at:

  setup script (bash)         /home/<user>/.conda/envs/cantera-dev/bin/setup_cantera
  setup script (csh/tcsh)     /home/<user>/.conda/envs/cantera-dev/bin/setup_cantera.csh

It is recommended that you run the script for your shell by typing:

  source /home/<user>/.conda/envs/cantera-dev/bin/setup_cantera

before using Cantera, or else include its contents in your shell login script.

scons: done building targets.

I am unfortunately not able to test on OSX, but as long as the file structure follows *nix, this should work.

bryanwweber

OK, one last round of suggestions here, I hope. Now that we've sorted out the set issue 😄

SConstruct

ischoegl · 2022-02-11T03:50:35Z

@bryanwweber ... thanks for the suggestions! I adopted all. I further double-checked that things still work as expected on Windows, and the Linux variant is taken care of by the Sundials runners. Should be good to go!

bryanwweber · 2022-02-11T20:17:44Z

@bryanwweber ... I think in this case, the safest option is to not replicate conda_prefix in the installation path at all. I.e. if you specify stage_dir=conda without a prefix then things end up in <cantera_src_dir>/conda/...

Would this work? (just pushed a version that should accomplish that.)

What I want is to replicate the exact conda installation paths inside the stage_dir so that I can verify that it all looks correct. So your most recent change is also going to not quite accomplish the right thing, since the conda path structure is not set as the prefix when stage_dir is set.

ischoegl · 2022-02-11T20:30:50Z

What I want is to replicate the exact conda installation paths inside the stage_dir so that I can verify that it all looks correct.

That's easy on *nix, but will fail on windows. Regarding the other issue, that should be fixed. If you have a robust suggestion for a (truncated) prefix generated from conda_prefix, we can likely put this to rest. What about the last three 'parents', i.e.

stage/.conda/envs/cantera-dev (*nix) or
stage\miniconda3\envs\cantera-dev (windows)?

ischoegl · 2022-02-11T23:58:58Z

ok. ran a test for scons build stage_dir=stage on windows, with the following output

postInstallMessage(["finish_install"], [])

Cantera has been successfully installed.

File locations:

  applications                C:\Users\<user>\GitHub\cantera\stage\miniconda3\envs\cantera-dev\Scripts
  library files               C:\Users\<user>\GitHub\cantera\stage\miniconda3\envs\cantera-dev\Library\lib
  C++ headers                 C:\Users\<user>\GitHub\cantera\stage\miniconda3\envs\cantera-dev\Library\include
  samples                     C:\Users\<user>\GitHub\cantera\stage\miniconda3\envs\cantera-dev\share\cantera\samples
  data files                  C:\Users\<user>\GitHub\cantera\stage\miniconda3\envs\cantera-dev\share\cantera\data
  Python package (cantera)    \Users\<user>\miniconda3\envs\cantera-dev\Lib\site-packages
  Python samples              \Users\<user>\miniconda3\envs\cantera-dev\Lib\site-packages\cantera\examples

scons: done building targets.

So it does look like the python packages aren't redirected as required.

Edit: Python Package is not installed in correct location, but

C:\Users\<user>\GitHub\cantera\stage\Users\<user>\miniconda3\envs\cantera-dev\Lib\site-packages\cantera

ischoegl · 2022-02-12T00:46:37Z

@bryanwweber / @speth ... I ran some tests but am not completely following why the logic used for setting 'python_module_loc' fails when stage_dir is set (on Windows). I'm not seeing what the conda layout does differently for things to break? ~~I'm wondering whether this is OS specific?~~

... update ...

postInstallMessage(["finish_install"], [])

Cantera has been successfully installed.

File locations:

  applications                /work/GitHub/cantera/stage/.conda/envs/cantera-dev/bin
  library files               /work/GitHub/cantera/stage/.conda/envs/cantera-dev/lib
  C++ headers                 /work/GitHub/cantera/stage/.conda/envs/cantera-dev/include
  samples                     /work/GitHub/cantera/stage/.conda/envs/cantera-dev/share/cantera/samples
  data files                  /work/GitHub/cantera/stage/.conda/envs/cantera-dev/share/cantera/data  
  Python package (cantera)    /.conda/envs/cantera-dev/lib/python3.10/site-packages
  Python samples              /.conda/envs/cantera-dev/lib/python3.10/site-packages/cantera/examples

Setup scripts to configure the environment for Cantera are at:

  setup script (bash)         /work/GitHub/cantera/stage/.conda/envs/cantera-dev/bin/setup_cantera
  setup script (csh/tcsh)     /work/GitHub/cantera/stage/.conda/envs/cantera-dev/bin/setup_cantera.csh

It is recommended that you run the script for your shell by typing:

  source /work/GitHub/cantera/stage/.conda/envs/cantera-dev/bin/setup_cantera

before using Cantera, or else include its contents in your shell login script.

scons: done building targets.

Edit: Python is installed in the expected locations, although they are not displayed correctly

ischoegl · 2022-02-12T02:37:29Z

Alright ... mystery solved: on Linux, the stage_dir option works, but the wrong Python installation folders are displayed in the post-install message. On Windows, the Python module is installed into the stagedir, but not correctly. I believe this due to how python_module_loc is handled. At this point, this PR fixes most of the displayed installation locations, but the Python issue runs deeper.

On windows, the installation folder for the Python package is

 C:\Users\ischo\GitHub\cantera\stage\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages\cantera

Also, add conda library/include paths (if applicable)

ischoegl · 2022-02-12T03:58:18Z

@bryanwweber / @speth ... I just opened #1194 as the issues with the 'stage' location opens a can of worms that goes well beyond the scope of what this PR intended to achieve. I rolled back all the 'fixes' for the post-build message, as I believe that they need to be addressed in another PR with a more narrow scope.

In a nutshell:

Installation with the conda layout works without restrictions if no stage_dir is used
If a stage_dir is used, installation works on *nix although installation locations are displayed incorrectly
On Windows, there are issues with the Python package installation

There are two reasons why I'd like to defer a full resolution: (i) #1158 may change some of the behavior, and (ii) having the conda layout available will provide for a convenient means to resolve #1194.

That said, I believe the PR is ready with the caveats stated above.

speth

Thanks for implementing this, @ischoegl. I think this will simplify from-source installation a lot in one of the most common use scenarios. My suggestions here are mostly around expanding the documentation of this feature.

.github/workflows/main.yml

SConstruct

ischoegl · 2022-02-14T16:20:48Z

@speth ... thank you for the suggestions - these were all easily adopted. While the Sundials runner does detect all system installs correctly, I agree that it's safer to leave the explicit options in.

@bryanwweber ... unless you have any other suggestions, I believe this is ready to merge!

ischoegl force-pushed the conda-prefix branch from 63b7d27 to c4f382d Compare February 8, 2022 22:00

ischoegl marked this pull request as ready for review February 8, 2022 22:17

ischoegl requested a review from a team February 8, 2022 22:17

ischoegl force-pushed the conda-prefix branch 2 times, most recently from 7a2e1fd to 1698d80 Compare February 8, 2022 22:42

ischoegl added compiling enhancement labels Feb 8, 2022

ischoegl mentioned this pull request Feb 8, 2022

Applications fail to install in bin on Windows #1192

Closed

ischoegl force-pushed the conda-prefix branch 3 times, most recently from 13ebd52 to b0cee4f Compare February 9, 2022 00:12

bryanwweber reviewed Feb 9, 2022

View reviewed changes

SConstruct Outdated Show resolved Hide resolved

ischoegl force-pushed the conda-prefix branch 3 times, most recently from 8f1d589 to de362a9 Compare February 10, 2022 00:34

bryanwweber requested changes Feb 10, 2022

View reviewed changes

SConstruct Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

ischoegl force-pushed the conda-prefix branch 4 times, most recently from a9c1748 to fd5fb32 Compare February 10, 2022 14:52

bryanwweber self-requested a review February 10, 2022 15:55

ischoegl force-pushed the conda-prefix branch 2 times, most recently from efcf06f to a19de56 Compare February 10, 2022 17:57

bryanwweber requested changes Feb 11, 2022

View reviewed changes

bryanwweber self-requested a review February 11, 2022 14:04

speth self-requested a review February 11, 2022 18:11

ischoegl force-pushed the conda-prefix branch from 30e90fc to 4e9c2b9 Compare February 11, 2022 20:23

ischoegl force-pushed the conda-prefix branch 2 times, most recently from 134b217 to c735879 Compare February 11, 2022 23:55

ischoegl force-pushed the conda-prefix branch from c735879 to 9d9b48b Compare February 12, 2022 00:43

ischoegl force-pushed the conda-prefix branch from 9d9b48b to 79e53d5 Compare February 12, 2022 02:42

[SCons] Change prefix default if conda is detected

e182274

Also, add conda library/include paths (if applicable)

ischoegl force-pushed the conda-prefix branch from 79e53d5 to b749aa3 Compare February 12, 2022 03:27

ischoegl force-pushed the conda-prefix branch from b749aa3 to 15aa82a Compare February 12, 2022 04:00

This was referenced Feb 12, 2022

Installation with stage_dir displays incomplete/wrong locations #1194

Closed

Add package workflow #1158

Merged

speth requested changes Feb 14, 2022

View reviewed changes

.github/workflows/main.yml Outdated Show resolved Hide resolved

SConstruct Show resolved Hide resolved

SConstruct Outdated Show resolved Hide resolved

SConstruct Show resolved Hide resolved

ischoegl added 4 commits February 14, 2022 09:41

[CI] Remove extra_*_dirs option from conda installed runner

9ba8603

[SCons] Simplify implementation per review comments

cdbe6f3

[SCons] Redirect conda destination if 'stage_dir' is specified

c30e583

[SCons] Improve docstrings of options affected by the 'conda' layout

58be58a

ischoegl force-pushed the conda-prefix branch from 15aa82a to 58be58a Compare February 14, 2022 16:11

ischoegl requested a review from speth February 14, 2022 16:23

speth approved these changes Feb 14, 2022

View reviewed changes

bryanwweber approved these changes Feb 15, 2022

View reviewed changes

bryanwweber merged commit 0b9eae3 into Cantera:main Feb 15, 2022

ischoegl deleted the conda-prefix branch February 15, 2022 12:53

ischoegl mentioned this pull request Mar 24, 2022

Document compilation in conda environment Cantera/cantera-website#181

Closed

ischoegl mentioned this pull request Aug 10, 2022

Fix windows and cantera-matlab builds Cantera/conda-recipes#34

Merged

Uh oh!

Conversation

ischoegl commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ischoegl commented Feb 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Feb 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bryanwweber commented Feb 11, 2022

Uh oh!

ischoegl commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ischoegl commented Feb 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

speth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ischoegl commented Feb 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ischoegl commented Feb 8, 2022 •

edited

Loading

codecov bot commented Feb 8, 2022 •

edited

Loading

ischoegl commented Feb 9, 2022 •

edited

Loading

ischoegl commented Feb 10, 2022 •

edited

Loading

ischoegl commented Feb 11, 2022 •

edited

Loading

ischoegl commented Feb 11, 2022 •

edited

Loading

ischoegl commented Feb 11, 2022 •

edited

Loading

ischoegl commented Feb 12, 2022 •

edited

Loading

ischoegl commented Feb 12, 2022 •

edited

Loading

ischoegl commented Feb 12, 2022 •

edited

Loading