raspberrypi / userland

Source code for ARM side libraries for interfacing to Raspberry Pi GPU.
BSD 3-Clause "New" or "Revised" License
2.04k stars 1.09k forks source link

buildme has unexpected side-effects! #546

Open lurch opened 5 years ago

lurch commented 5 years ago

I ran buildme on a Pi, and was surprised to see that at the end it was modifying system files (when there's no warning about this in the README). Yikes. I had a look at the source of buildme and saw it does:

if [ $ARCH = "armv6l" ] || [ $ARCH = "armv7l" ] || [ $ARCH = "aarch64" ]; then
  ... stuff ...
  sudo make install

IMHO unconditionally running a sudo make install (which then overwrites system-installed files) from inside a build-script run as a user is a bit nasty. I think it would be better if there were separate build and install steps, or if the install was only done if a specific flag was passed to the buildme script.

6by9 commented 5 years ago

It's been that way since day 0, so changing it now would be a bad thing. If make installed stuff automagically then I'd agree, but having a build script install stuff less so.

Update of README would be reasonable if you felt that strongly.

lurch commented 5 years ago

Given that buildme is the only documented way to build stuff in this repo, and you don't want to break backwards-compatibility, would you have any objection to having a buildme option to disable the "auto-install" feature?

6by9 commented 5 years ago

An option to disable the auto-install would be fine.

Tombana commented 5 years ago

I have also stumbled on this problem in the past, accidentally overwriting my system files when I did not want to. The first thing I do when cloning userland is to remove that line from buildme, so I would agree that it is good to warn for this in README and give a command line option to disable it.

Not 100% related, but also regarding the buildme script: in older versions, the script used uname -m which is now replaced by the command arch. On my raspberry pi, however, this command does not exist (Arch linux, ironically) so buildme didnt work. Is there a reason for using for not using uname -m anymore?

6by9 commented 5 years ago

Always open to PRs.

in older versions, the script used uname -m which is now replaced by the command arch

Really? The use of arch was added in https://github.com/raspberrypi/userland/commit/775cf0bb88aca0c4a5a471256019895970c06fdf#diff-f3c2f00f0ce631b8bd728603c4f30ac1. There were no architecture based build options before that.

https://github.com/raspberrypi/userland/commit/8b8f6571b67e36f28d00fb38dfaa3311d4faba13#diff-f3c2f00f0ce631b8bd728603c4f30ac1 I have no objections to switching to uname -m if someone wishes to raise a PR.

lurch commented 5 years ago

An option to disable the auto-install would be fine.

Always open to PRs.

If I find time next week I'll dig out the patch I created for this and submit a PR.

Tombana commented 5 years ago

Really? The use of arch was added in 775cf0b#diff-f3c2f00f0ce631b8bd728603c4f30ac1. There were no architecture based build options before that.

You are right, I think I changed it to uname -m locally and then forgot that I did that.

8b8f657#diff-f3c2f00f0ce631b8bd728603c4f30ac1 I have no objections to switching to uname -m if someone wishes to raise a PR.

Will do!

imerso commented 4 years ago

Well. I have been developing small things on Raspberry, but never got interested to check its pre-built tools and sources.

I had GL code which ran perfectly on this RPI here, for years...

Yesterday I decided to download this repo and run the "buildme"...

Now, NOTHING works, even the built-in tools like raspivid and raspistill.

I suppose that I will have to backup the sdcard and reinstall everything from scratch? Great work, thank you!

JamesH65 commented 4 years ago

What version of the original OS are you using? This repo will need a fairly recent installation of the OS in order to work correctly.

pelwell commented 4 years ago

@imerso I wouldn't have a problem with changing the script to print something like "Now run 'sudo make install' to install the build products", but I suspect that even if that had been the case you would probably have ended up in the same situation as you are now, i.e. be annoyed that it didn't work, not that you didn't get a choice.

imerso commented 4 years ago

I was annoyed because the script broke a working system in a seemingly irreversible way. I then had to take a backup of the sdcard (which includes network config, personal directories, programs etc), reflash the OS image, then copy all important things back, and do all the initial configuration again.

I did not really mean to be harsh, the rpi and all these built-in tools are nice. But I still dare to disagree that a "build" script that can potentially break a working system should be seen as normal and good practice. IMHO, a "build" script should do just that: build the code. If I ran an "install" script, then I could understand the result.

I sincerely believe that you should seriously consider disabling that automatic make install -- it definitely can break working systems, and should not. That is not a really good practice.

Thank you.

pelwell commented 4 years ago

I am open to the idea of changing it. @6by9 - would you consider reversing your decision, provided the new script a) had a way to force an install, and b) ended with a suitable message indicating that an extra step was required?

6by9 commented 4 years ago

I'm curious now. What would your next step have been once you built the libraries and apps? 99.9% of people would be wanting to use them, generally don't mess with finding the apps from a build directory, and therefore wish to install them.

Nothing built from userland should prevent a system from booting. sudo apt install --reinstall libraspberrypi (if I've got my packaging right) should reinstall anything that it overwrote.

rm -rf / can break a working system. Remove rm?

@lurch had written

If I find time next week I'll dig out the patch I created for this and submit a PR.

I don't know whether he couldn't find that patch or just forgot.

If a PR is raised then it'll be considered, however I'm very aware that there are a huge number of forum posts, blogs, and other places that all say

git clone https://github.com/raspberrypi/userland
cd userland
./buildme

that will be incorrect if the buildme script is changed to not install. People will take a set of instructions blindly and not read anything it prints out. The output from cmake and make as triggered from buildme is fairly verbose, so people won't read anything tagged on at the end.

imerso commented 4 years ago

My next step would be to execute them in place and then either modify raspivid code or integrate parts of it to a specific tool I have written already.

In neither case I would want to replace any of the existing (and working) system files.

I find this very natural, though, that before doing any code changes, a simple build is done. Now, I could have checked the script and finded that it would mess with the working tools (rendering them unusable), but should I really expect that from a "build" script?

Also the comparison with issuing a rm command is absurd. No one should be forced to review all your scripts or code to make sure they won't destroy the system, unless they're called "format", "install" or something like that.

Anyway, that's ok. I will start to review anything I download from this repo then, maybe there is indeed a rm hidden into something called "run", who knows?

Leo3418 commented 4 years ago

Hello maintainers, if you are still interested in getting rid of sudo in the build script, then I would suggest allowing users to specify a custom installation path rather than /opt/vc.

A good free software project is encouraged to honor options that customize installation path, such as cmake -DCMAKE_INSTALL_PREFIX=/usr/local. This provides several benefits, with one of them being that, if someone does not have superuser privilege but still want to build and install the software, they can install it to somewhere they can write to, like a path under their own home directory, with cmake -DCMAKE_INSTALL_PREFIX=~/vc. In this way, sudo is not required for make install at all.

Some additional benefits of allowing custom installation paths include:

Currently, the /opt/vc installation path is hard-coded into the build files and scripts, and specifying DCMAKE_INSTALL_PREFIX for cmake has no effect at all, preventing users from installing the program to somewhere else.

JamesH65 commented 4 years ago

It's odd that this broke the system - it doesn't build any system files, only userland applications, none of which could stop a Pi from booting. Depending on the age of the system, you might break raspistill/vid etc if they are looking for newer firmware, but even that seems unlikely, unless you have a really old system.

Since this is the first time a breakage has ever been flagged up in quite a few years, something odd seems to have happened.