dynamically resolve environment variables

zhongxiang117 commented 3 months ago

Description

This PR is to use solve the problem mentioned in #1086 section 2 about the dynamical link of "Plumed.h" header file.

Type of contribution

[ ] changes to code or doc authored by PLUMED developers, or additions of code in the core or within the default modules
[ ] changes to a module not authored by you
[X] new module contribution or edit of a module authored by you

Copyright

[X] I agree to transfer the copyright of the code I have written to the PLUMED developers or to the author of the code I am modifying.

Idea for update

I thoroughly go over the plumed=patch codes, after compilation, I found the call sequence of plumed=patch is:

a) plumed patch -p
    1) bin/plumed                       # plumed executor
    2) lib/plumed/scripts/patch.sh      # script interface
    3) lib/plumed/patches/patch.sh      # real codes do patch

b) plumed-patch -p
    1) bin/plumed-patch                 # plumed-patch executor
    2) lib/plumed/scripts/patch.sh      # script interface
    3) lib/plumed/patches/patch.sh      # real codes do patch

Be aware of that call a and b are different, a is run plumed first, and then use its internal solver to find patch codes, while b is directly run patch script.

The difference is environment variable PLUMED_INCLUDEDIR is hard-coded inside plumed binary object, where inside plumed-patch script, it can be customized.

Thus, to make it work, I made edition of two files, 1) src/lib/Makefile: add some lines to check environment variables before scripts executes, relating to b. 2) patches/patch.sh: to exclusively check PLUMED_INCLUDEDIR validation, relating to a.

I think the explanations should be clear, if there are any opaques, please let me know, thanks!

GiovanniBussi commented 3 months ago

Hi, thanks for the contribution. I am not sure this is robust. If I understand correctly:

You check if the usual path (the one used now) exist and if so you use that
If not, you use PLUMED_ROOT/../../include

This is fragile for a number of reasons:

If another version of plumed has been already installed on that path, the solver will use that
You are assuming that include dir is identical to prefix/include, and that libdir is identical to prefix/X/Y (with X Y arbitrary directories). This is not enforced anywhere else I think, and in some distributions it is not true.

Regarding your comments, let me add that:

You are not supposed to use the patches/patch.sh script directly. This is very much like the scripts present in scripts. It should be called via a wrapper that setups the environment correctly. Let me know if there is any documented use of the patches/patch.sh script, which should be fixed.
Even though the path to include files is hard coded in the binary executable, it can still be adjusted. Did you try to set PLUMED_INCLUDEDIR before running plumed?
Using PLUMED_ROOT to adjust this is a last resort.

I am still a bit confused about what you are trying to provide. Something like this?

install plumed to a prefix
move it somewhere else
set some env var to consider the new path
patch an MD code
compile the MD code

I think that the solution for relocatability is to use:

'dladdr` in the cpp code that I linked above to know where the library is
some bash trick in the bash wrappers to find the path of "this" executing script

This will still have the open issues that I mentioned here. But it would allow you to relocate plumed and use it (without setting and env var), though on Linux only.

zhongxiang117 commented 3 months ago

Thanks for your comments. Now I know where your confusion is, before that let me explain the things you have talked about.

1) If PLUMED_ROOT/../../include is dangerous, I can normalize that by using realpath command. 2) I am not directly using patches/patch.sh script, the above writing is to show the call sequences when the script under bin folder executes. After compilation and installation, there are three files under bin folder, which is supposed to be added to PATH environment variable, then user can have access to plume, plume-config, and plume-patch commands. So I tested the differences between plume and plume-patch scripts, and found that they do have differences.

This is not about on the installation stage, installation can be anywhere as long as configurations, ./configure, success, no matter whether --prefix or DESTDIR used or not.

What I am trying to provide is to make the environmental setting more concise and standard, I mean usually the program will be compiled and installed in a standard way, like installing to PREFIX/bin, PREFIX/lib, and etc.. Seldom, normally for advanced users, the files will be separately installed to PREFIX_A/bin, PREFIX_B/lib, and etc. customizing each type of object paths.

So after installation, even for different versions of program, the compiled files under like PREFIX/bin, PREFIX/lib, and so on, the paths of them will be corresponded to each other. Thus, the way I provide is to by only setting “PROGRAM_ROOT” once, all other its same-time compiled files can be relocated automatically, when using patch scripts.

I know by additionally setting PLUMED_INCLUDEDIR to different and the “same-time” installed folder works and meets my needs, but I want to solve it in a more standard way by only using PLUMED_ROOT variable, and I don't find any docs talking about this, if has, please enlighten me and let me know.

More important, the update only take effects on patch scripts, and make it more robust to find include headers, it will not influence how bin/plumed executable to relocate configuration files or static library(if compiled in this way), so your worries are not a problem.

GiovanniBussi commented 3 months ago

Ok let me try again to understand what you are trying to do.

install plumed somewhere
move the installed package somewhere else
export PLUMED_ROOT and fix PATH
use plumed patch to patch an MD code

With the current implementation, if you skip step 2 everything works as expected, correct?

At point 4 you are only interested in plumed patch, correct?

If so, I guess your solution should not work. Even if you fix the paths seen in the patch.sh script so that the links work correctly, the make and cmake include files will still contain absolute paths. Please have a look at files $prefix/lib/plumed/src/lib/Plumed.cmake and $prefix/lib/plumed/src/lib/Plumed.make.

The only solution, to my understanding, is to:

modify the patch script so that it copies those files rather than providing links
while copying those files, edit them on the fly to adjust absolute paths

Alternatively, if possible, modify the Plumed.cmake and Plumed.make files so that they can automatically find their own location when included. I know this is possible with bash scripts (see e.g. here). I don't know if it's possible for make and cmake files.

Related to the problem of having include directory not equal to plumed_root/../../include, I think the clean solution is to find the correct relationship as a relative path. I guess you can do it at run time, when the directories exist already.

zhongxiang117 commented 3 months ago

Hi @GiovanniBussi, your understanding of what I am trying to do is correct. I should have explained it at the very beginning.

I know where is your worry about, it is not about the compilation stage, or any make or cmake files.

The PR of new codes just adds an additional layer when running plumed patch or plumed-patch, or other BASH scripts, it will help find the correct PLUEMD_INCLUDEDIR path.

Let us assume the compilation has been done, the plumed program has been successfully created with all its needed files and objects into a compact folder, and we know some paths inside bin/plumed binary uses absolute path, and it works as expect.

Now, I move this folder to another place, and correctly setting PLUMED_ROOT to that new path, the plumed still works(this is very important), but patches cannot find its correct header file(this is the problem that I encountered).

Thus, I provided a way to solve it, by firstly checking the validation of the folder path before doing really patch. It will not influence all other settings, it simply adds a second layer to help locate header files. If no folder path changes, those codes will be never run.

To conclude, there are two very important things that I should mention, to relieve any of your concerns.

1) The codes in PR only adds a warranty to help set the correct PLUMED_INCLUDEDIR path when something happens 2) bin/plumed will work as always, no matter its path of folder renamed or moved to other place (I have tested it, 100% sure)

zhongxiang117 commented 3 months ago

One more thing, the "standard" way to solve the relative path plumed_root/../../include, the same solution can be used as referenced from configure file, by using realpath during its runtime.

Also, I see all those scripts are generated and assumed to be run #!/usr/bin/env bash environment, no other type SHELLs are defined, thus, I do not think it is a big problem.

GiovanniBussi commented 3 months ago

I see that your patch is not affecting the rest of the code. It is just making plumed patch -p able to properly patch MD codes after plumed has been moved, adding symbolic links to the correct files. To my understanding, however, it will not be possible to compile the patched code (because the cmake and make include files still contain the absolute path to the location of plumed before it was moved). Thus, I don't think it is worth merging it.

I am closing this but feel free to reopen if there's something I misunderstood.

plumed / plumed2