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

Installation with stage_dir displays incomplete/wrong locations #1194

Closed
ischoegl opened this issue Feb 12, 2022 · 8 comments
Closed

Installation with stage_dir displays incomplete/wrong locations #1194

ischoegl opened this issue Feb 12, 2022 · 8 comments

Comments

@ischoegl
Copy link
Member

ischoegl commented Feb 12, 2022

Problem description

An installation with stage_dir on windows produces the following behavior (the behavior is presumably worse on debian):

(cantera-dev) PS C:\Users\ischo\GitHub\cantera> scons install stage_dir=stage
[...]
postInstallMessage(["finish_install"], [])

Cantera has been successfully installed.

File locations:

  applications                C:\Program Files\Cantera\bin
  library files               C:\Program Files\Cantera\lib
  C++ headers                 C:\Program Files\Cantera\include
  samples                     C:\Program Files\Cantera\samples
  data files                  C:\Program Files\Cantera\data
  Python package (cantera)    \Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages
  Python samples              \Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages\cantera\examples
scons: done building targets.
(cantera-dev) PS C:\Users\ischo\GitHub\cantera> cd stage
(cantera-dev) PS C:\Users\ischo\GitHub\cantera\stage> ls

    Directory: C:\Users\ischo\GitHub\cantera\stage

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           2/11/2022  9:42 PM                Program Files
d----           2/11/2022  9:43 PM                Users

(cantera-dev) PS C:\Users\ischo\GitHub\cantera\stage> ls '.\Program Files\Cantera\'

    Directory: C:\Users\ischo\GitHub\cantera\stage\Program Files\Cantera

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           2/11/2022  9:42 PM                data
d----           2/11/2022  9:42 PM                doc
d----           2/11/2022  9:42 PM                include
d----           2/11/2022  9:43 PM                lib
d----           2/11/2022  9:43 PM                samples

(cantera-dev) PS C:\Users\ischo\GitHub\cantera\stage> ls .\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages\c*

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

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           2/11/2022  9:43 PM                cantera
d----           2/11/2022  9:43 PM                Cantera-2.6.0a4-py3.9.egg-info

(cantera-dev) PS C:\Users\ischo\GitHub\cantera\stage>

Steps to reproduce

  1. Compile from source
  2. Install with stage_dir option
  3. See faulty installation above

Expected Behavior

The full paths (including stage_dir) should be displayed.

System information

  • Cantera version: 2.6.0a4 (main / 5b58690)
  • OS: Windows 10
  • Python/MATLAB/other software versions: miniconda, Python 3.9.7

Additional context

@ischoegl ischoegl changed the title Installation with stage_dir displays wrong locations / fails on Windows Installation with stage_dir displays wrong locations Feb 12, 2022
@ischoegl ischoegl changed the title Installation with stage_dir displays wrong locations Installation with stage_dir displays incomplete/wrong locations Feb 12, 2022
@bryanwweber
Copy link
Member

Can you show your cantera.conf for this setup?

As far as I understand, it's intentional that the stage_dir is not shown in front of the install paths in the output. I suppose the reason is that it will be the same for all of them and also not relevant to the installed package.

That said, I doubt stage_dir has been extensively tested on Windows. So I wouldn't be surprised if there are problems.

As far as python_module_loc, that will be changing when we move to pip to install, so I wouldn't worry too much about that here.

@ischoegl
Copy link
Member Author

Can you show your cantera.conf for this setup?

stage_dir = 'stage'
boost_inc_dir = 'C:/Users/ischo/miniconda3/envs/cantera-dev/Library/include'

As far as I understand, it's intentional that the stage_dir is not shown in front of the install paths in the output. I suppose the reason is that it will be the same for all of them and also not relevant to the installed package.

ok. but the Python formatting is not consistent. It may also make sense to state the stage location and strip the C:\ from the paths.

That said, I doubt stage_dir has been extensively tested on Windows. So I wouldn't be surprised if there are problems.

It technically works, but the implementation is not robust.

As far as python_module_loc, that will be changing when we move to pip to install, so I wouldn't worry too much about that here.

I'm aware of this, but this issue keeps me from addressing stage_dir in #1191. Imho it would make sense to fix this issue after #1158 is merged.

Fwiw, I rolled back all display 'fixes' in #1191 in order to strictly stick to the original intent (I believe it is ready to merge with the caveat that a staged Python install doesn't work for Windows, and that the install locations are not displayed correctly on *nix).

@bryanwweber
Copy link
Member

ok. but the Python formatting is not consistent.

Yes, but this will be changing in #1158.

It may also make sense to state the stage location and strip the C:\ from the paths.

Stripping the C:\ from the path is already done if the stage directory is absolute, see:

cantera/SConstruct

Lines 1670 to 1676 in 5b58690

if env["stage_dir"]:
stage_prefix = Path(env["prefix"])
# Strip the root off the prefix if it's absolute
if stage_prefix.is_absolute():
stage_prefix = Path(*stage_prefix.parts[1:])
instRoot = Path.cwd().joinpath(env["stage_dir"], stage_prefix)
I agree it would make sense to show the stage directory.

and that the install locations are not displayed correctly on *nix

At least on my Mac, they appear to be shown correctly 🤷‍♂️ If I set prefix=/Users/bryan/Documents stage_dir=stage I get locations that just start with the prefix.

Imho it would make sense to fix this issue after #1158 is merged.

Sure, that's a good idea, or else as part of #1158

@ischoegl
Copy link
Member Author

ischoegl commented Feb 14, 2022

Stripping the C:\ from the path is already done if the stage directory is absolute [...]

Not sure that it's working on windows, see output listed on top.

At least on my Mac, they appear to be shown correctly

I think if you consider that stage_dir is not shown, then it also works on Linux. But not on Windows (see above). Edit: the installation locations on debian are also incorrect.

Overall, I'm really looking forward to #1158, which I believe will bring a more robust Python installation!

@ischoegl
Copy link
Member Author

After merging #1158, the post-install message for a build with scons build -j4 stage_dir=C:\Users\ischo\stage and scons install is

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

    Cantera has been successfully installed.

    File locations:
      library files               miniconda3\envs\cantera-dev\Library\lib
      C++ headers                 miniconda3\envs\cantera-dev\Library\include
      samples                     miniconda3/envs/cantera-dev\share\cantera\samples
      data files                  miniconda3/envs/cantera-dev\share\cantera\data
      input file converters       C:\Users\ischo\miniconda3\envs\cantera-dev\Scripts
      Python package              C:\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages
      Python examples             C:\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages\cantera\examples

scons: done building targets.

with the following folders in the stage directory:

(cantera-dev) PS C:\Users\ischo\stage> ls

    Directory: C:\Users\ischo\stage

Mode                 LastWriteTime         Length Name
----                 -------------         ------ ----
d----           3/23/2022  3:41 PM                miniconda3
d----           3/23/2022  3:42 PM                Users

I'm honestly not quite sure what the intended path would be. Perhaps the stage_dir folder doesn't make much sense on Windows ...

@bryanwweber
Copy link
Member

The absolute folders for the Python-related stuff is likely a result of how those paths are determined via pip. That might be able to be improved, although I'm not keen on trying to work it out without access to a Windows box.

The other paths seem like a bad interaction between conda and the stage_dir to me. But they are exactly what I would expect. If you didn't have stage_dir, files would be installed to C:\Users\ischo\<remainder of the shown path>. So relative to the stage_dir, that's the rest of the path you'd expect to see.

@ischoegl
Copy link
Member Author

I'm actually not really concerned about windows at this moment any longer, now that the prefix issue is fixed for good. Fwiw, this is likely unused and thus not worth fixing (plus I agree, the paths correspond to what is displayed). As long as refactoring for 2.6 didn't break anything, this is not really an issue ...

@bryanwweber
Copy link
Member

Great, I'll close this issue then 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants