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

Make Erlang installation file system location independent #5427

Conversation

kjellwinblad
Copy link
Contributor

@kjellwinblad kjellwinblad commented Nov 19, 2021

Previously shell scripts (e.g., erl and start) and the RELEASES file
for an Erlang installation depended on hard coded absolute path to the
installation's root directory. This made it cumbersome to move an
installation to a different directory which can be problematic for platforms
such as Android (#2879) where the
installation directory is unknown at compile time. This is fixed by:

  • Changing the shell scripts so that they can dynamically find the
    ROOTDIR. The dynamically found ROOTDIR is selected if it differs
    from the hard-coded ROOTDIR and seems to point to a valid Erlang
    installation. The dyn_erl program has been changed so that it can
    return its absolute canonicalized path when given the --realpath
    argument (dyn_erl gets its absolute canonicalized path from the
    realpath POSIX function). The dyn_erl's --realpath
    functionality is used by the scripts to get the root dir dynamically.

  • Changing the release_handler module that reads and writes to the
    RELEASES file so that it prepends code:root_dir() whenever it
    encounters relative paths. This is necessary since the current
    working directory can be changed so it is something different than
    code:root_dir().

@kjellwinblad
Copy link
Contributor Author

kjellwinblad commented Nov 19, 2021

This PR solves the same problem that PR #2879 tries to solve but handles absolute paths in the RELEASES file (that is used by the release_handler module) as well, and uses a different method to obtain the $ROOTDIR dynamically from the scripts (erl, start and cerl).

@rickard-green rickard-green added the team:VM Assigned to OTP team VM label Nov 19, 2021
@JeromeDeBretagne
Copy link
Contributor

I have only reviewed the modified shell scripts, this is looking great! I've built a cross-compiled version for Android and the ERL_ROOTDIR environment variable is still working too 👍

Not totally related to your changes but while reviewing the scripts, I'm still wondering why the erl scripts are using :

PROGNAME=`echo $0 | sed 's/.*///'`

and not directly :

PROGNAME=erl

Similarly in start_erl :

PROGNAME=start_erl

I may be missing an interesting usage but as there is no comment in the code, I'm wondering if this is simply a leftover that could be simplifed by now. Any ideas @kjellwinblad ?

@garazdawi
Copy link
Contributor

I may be missing an interesting usage but as there is no comment in the code, I'm wondering if this is simply a leftover that could be simplifed by now.

I'm only guessing here, but wouldn't it allow the user to rename the file to anything it wants and init:get_argument(progname) would then reflect that name.

This would make release upgrades and other things just work without having to modify the erl script if you want to call it something different.

@JeromeDeBretagne
Copy link
Contributor

I'm only guessing here, but wouldn't it allow the user to rename the file to anything it wants and init:get_argument(progname) would then reflect that name.

Indeed, this is a pattern that is even described in the init documentation.

This would make release upgrades and other things just work without having to modify the erl script if you want to call it something different.

This may be used this way, thanks for sharing your guess. I was wondering if we could get rid of sed as it wasn't supported in earlier versions of Android (it was only introduced in Android 6.0 Marshmallow). Too bad it's not possible but now at least I have a better understanding of this part of the scripts.

@garazdawi
Copy link
Contributor

I've pushed a change that adds release_handler:create_RELEASES/3 which is the same as the previous release_handler:create_RELEASES/4 with "" as first argument.

I also updated the move_system testcase to check the symlinks work as they should and to test that this all works on Windows.

I've also run all tests in rebar3, relx, and elixir and they pass, so at least what they test has not been broken.

An odd thing that I found in Windows was that if there is a file called erl.ini in the same folder as the erl.exe, then erl.exe will read its contents and use that as a way to find the root dir. But if you remove that file, then Windows managed to find the rootdir anyway... so the question is if that erl.ini file is still needed in some circumstances or if we can remove it...

@TheGeorge
Copy link
Contributor

We have a use-case, where the invoking program replaces arg0, which breaks the discovery logic.

The recommendation we got was to discover binaries similar to rusts current_exe logic (see here https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/os.rs#L352, note that the logic is OS dependent)

Is this PR aiming at addressing this problem as well?

@garazdawi
Copy link
Contributor

garazdawi commented Dec 13, 2021

We have a use-case, where the invoking program replaces arg0, which breaks the discovery logic.

The recommendation we got was to discover binaries similar to rusts current_exe logic (see here https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/os.rs#L352, note that the logic is OS dependent)

Is this PR aiming at addressing this problem as well?

I don't think we knew that it was a problem, so no, this PR does not try to solve that. Could you provide some more details about what it is that you are doing and why things break?

@garazdawi garazdawi force-pushed the kjell/erl/make_installation_location_independentR/OTP-17730 branch from ab4af23 to a50e8ad Compare December 13, 2021 10:15
kjellwinblad and others added 5 commits December 13, 2021 11:19
Previously shell scripts (e.g., `erl` and `start`) and the RELEASES file
for an Erlang installation depended on hard coded absolute path to the
installation's root directory. This made it cumbersome to move a
release to a different directory and can be problematic for platforms
such as Android (erlang#2879) where the
installation directory is unknown at compile time. This is fixed by:

* Changing the shell scripts so that they can dynamically find the
  `ROOTDIR`. The dynamically found `ROOTDIR` is selected if it differs
  from the hard-coded `ROOTDIR` and seems to point to a valid Erlang
  installation. The `dyn_erl` program has been changed so that it can
  return its absolute canonicalized path when given the `--realpath`
  argument (dyn_erl gets its absolute canonicalized path from the
  `realpath` POSIX function). The `dyn_erl`'s `--realpath`
  functionality is used by the scripts to get the root dir dynamically.

* Changing the `release_handler` module that reads and writes to the
  `RELEASES` file so that it prepends `code:root_dir()` whenever it
  encounters relative paths. This is necessary since the current
  working directory can be changed so it is something different than
  `code:root_dir()`.
@garazdawi garazdawi force-pushed the kjell/erl/make_installation_location_independentR/OTP-17730 branch from a50e8ad to 29a7158 Compare December 13, 2021 10:20
@TheGeorge
Copy link
Contributor

We have a use-case, where the invoking program replaces arg0, which breaks the discovery logic.
The recommendation we got was to discover binaries similar to rusts current_exe logic (see here https://github.com/rust-lang/rust/blob/master/library/std/src/sys/unix/os.rs#L352, note that the logic is OS dependent)
Is this PR aiming at addressing this problem as well?

I don't think we knew that it was a problem, so no, this PR does not try to solve that. Could you provide some more details about what it is that you are doing and why things break?

We use an elaborate type of symlink, but instead of referencing files in the filesystem, we reference other locations (think BLOB storage). The effect of this is, that the readlink to get to the "real" location of erl doesn't work, and OTP machinery thinks that erl is located where the "symlink" is.

Doing a readlink on /proc/self/exe works reliable (also for symlinks) at least on Linux. But a general robust self-discovery that is platform independent is more complicated (see the referenced rust code)

I just wanted to raise awareness for this. We currently add a shell script to our builds, that works around this issue, but it's one more trampoline that ideally we wouldn't need.

@garazdawi
Copy link
Contributor

We use an elaborate type of symlink, but instead of referencing files in the filesystem, we reference other locations (think BLOB storage). The effect of this is, that the readlink to get to the "real" location of erl doesn't work, and OTP machinery thinks that erl is located where the "symlink" is.

I assume that the same problem exists for realpath and not only readlink?

@TheGeorge
Copy link
Contributor

I assume that the same problem exists for realpath and not only readlink?

Yes, it's not an actual symlink, so realpath doesn't work.

@garazdawi garazdawi merged commit bfee11f into erlang:master Dec 22, 2021
@garazdawi
Copy link
Contributor

Finally merged! Thanks to everyone involved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team:VM Assigned to OTP team VM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants