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

Running make in a configured wasi-sdk tree installs things into /usr/local #442

Closed
whitequark opened this issue Jul 13, 2024 · 10 comments · Fixed by #446
Closed

Running make in a configured wasi-sdk tree installs things into /usr/local #442

whitequark opened this issue Jul 13, 2024 · 10 comments · Fixed by #446

Comments

@whitequark
Copy link
Contributor

I did not expect this behavior (normally only make install does that, which I was going to run with DESTDIR=, as usual) and it is fairly unwelcome in that I don't know how or if I can uninstall those files.

@whitequark whitequark changed the title Running make in wasi-sdk installs things into /usr/local Running make in a configured wasi-sdk tree installs things into /usr/local Jul 13, 2024
@whitequark
Copy link
Contributor Author

Double-checking my assumptions, the README suggests that the install target would install files, so my conclusion that the default target won't seems to be supported in that. I guess this is probably an oversight, but I still think it's an issue--just running make isn't supposed to modify the host system!

@sbc100
Copy link
Member

sbc100 commented Jul 13, 2024

I agree, only the install target(s) should put things in /usr/local. Is this something that change in the recent cmake transition? It not unlikely that there will be a few things like this to iron out.

BTW, unless you run a root copying stuff to /usr/local/ should give you permission denied (at least on linux systems I'm familiar with.. I don't know how this works on macOS). i.e. sudo should be required the install phase by default.

The problem of not being able to cleanup file installed by make install is fairly common. I think both cmake and autoconf suffer from this problem, sadly. They don't have make uninstall targets. I don't know the history of that be its always been that way I think. I guess package managers are they way that got solved.

@whitequark
Copy link
Contributor Author

Is this something that change in the recent cmake transition? It not unlikely that there will be a few things like this to iron out.

I think so.

BTW, unless you run a root copying stuff to /usr/local/ should give you permission denied (at least on linux systems I'm familiar with.. I don't know how this works on macOS). i.e. sudo should be required the install phase by default.

I have /usr/local configured to be owned by my $USER because it's essentially only used for software that's fussy about running from the build tree (no rpath, hardcoded resource paths, etc) and I'm the only one using this workstation.

On macOS this is done by default by Homebrew so this setup is actually quite common.

The problem of not being able to cleanup file installed by make install is fairly common. I think both cmake and autoconf suffer from this problem, sadly. They don't have make uninstall targets. I don't know the history of that be its always been that way I think. I guess package managers are they way that got solved.

Yes, which is why I put such an emphasis on not installing them there in the first place.

@alexcrichton
Copy link
Collaborator

This is sort of intended sort of not. With the previous Makefile you couldn't change the installation directory at all, it was simply always build/install. I wanted to add the option of changing things.

This is made difficult due to the dependencies between projects. For example if you want to build libcxx you have to previously build and install wasi-libc. It's not sufficient to simply build wasi-libc, it's got to get installed so clang knows how to pick it up. This all worked before because the installation directory was fixed to be inside of the build directory.

I'm happy to help implement other changes here, but the best I can think of is to either (1) error or warn on a missing -DCMAKE_INSTALL_PREFIX argument or (2) default the installation prefix to inside the build directory. Either that or documenting the current behavior.

I don't know how to architect things here such that it's a classic "run make to do all the build stuff and run make install to run all the copy stuff". Due to the usage of multiple projects I just don't know how to do that. If others know how though I'd be also happy to implement that.

@sbc100
Copy link
Member

sbc100 commented Jul 15, 2024

Its a little confusing here since we want to do installation-like things as part of our build.

The building and installing of things into the sysroot should probably all be done locally inside the build directory. (i.e. llvm and clang should be configure to install into a location that is inside of the wasi-sdk build directory).

Actualy running make install at the wasi-sdk level should either do nothing (error out) or it should install into subdirectory such as /usr/local/share/wasi-sdk/... it should not be installing its sysroot stuff like libs and header into the system header/library directories.

I think just having make install be an error or do nothing makes the most sense for now.

@sbc100
Copy link
Member

sbc100 commented Jul 15, 2024

Certainly typing make should not try to write to /usr/local/..

@whitequark
Copy link
Contributor Author

Either that or documenting the current behavior.

I think this is the only option that's really not okay as that's very easy to miss and basically no well-behaved software installs stuff into /usr when you type make.

alexcrichton added a commit to alexcrichton/wasi-sdk that referenced this issue Jul 15, 2024
This changes everything to ensure that only the `install` step actually
tries to install things. Everything is staged into temporary `./install`
folders inside of the build directory and then running the build
system's `install` target will actually copy out everything using CMake
builtins.

Closes WebAssembly#442
@alexcrichton
Copy link
Collaborator

I'll note I'm not defending the current state of affairs. My testing didn't surface this and I'm not a day-in-day-out C/C++ guru so this wasn't the first thing I looked to design when writing the CMake integration. I wanted to mostly outline the various options I thought were possible.

Regardless #446 should implement this.

@whitequark
Copy link
Contributor Author

Ah I see. #446 is perfect, thank you!

alexcrichton added a commit that referenced this issue Jul 16, 2024
* Update build of toolchain/sysroot to not touch installation root

This changes everything to ensure that only the `install` step actually
tries to install things. Everything is staged into temporary `./install`
folders inside of the build directory and then running the build
system's `install` target will actually copy out everything using CMake
builtins.

Closes #442

* Better integrate generating a version file
@whitequark
Copy link
Contributor Author

Thanks for fixing this again!

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

Successfully merging a pull request may close this issue.

3 participants