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

Cannot make install as non-root user #112

Open
0cjs opened this issue Apr 7, 2020 · 15 comments
Open

Cannot make install as non-root user #112

0cjs opened this issue Apr 7, 2020 · 15 comments

Comments

@0cjs
Copy link
Contributor

0cjs commented Apr 7, 2020

The Makefile's install target hardcodes ownership changes to root with commands like install -m 755 -o root -g root .... This prevents non-root installs under a different PREFIX "local" to a project, such as one might use for development.

I don't have any particuarly strong opinions about how to fix this. Perhaps we could set a variable that could be overridden? E.g., have INSTALL_OWNER = -o root -g root in the Makefile, allowing make install INSTALL_OWNER= on the command line to override this.

I'm happy to write and test the patch for whatever is deemed the correct solution.

@ThorstenBr
Copy link
Contributor

I don't think the "-o root -g root" is actually needed. Other makefiles (of other projects) generated by standard build tools like "autotools" / "./configure" simply call "install" without "-g" or "-o". In fact, file permissions are still set to "root" when running "sudo make install".

This is related to #73 though. The whole installation/packaging process could do with some clean-up / improvement. A standard build/install requires to control the output directory. Something like
make PREFIX=/foo/bar/MySeparateInstallDirectory install
should be supported.

This is also what most packaging environments expect. They usually expect a project to be able to build and install into a clean, separate install directory (by altering the PREFIX). And then they use this separate install directory to wrap up the installer package (Red Hat "rpm", Debian "deb", etc) - which is not how the current "make package" works.

@rhaleblian
Copy link
Contributor

Agreed, the chmods break non-root prefix use cases, as the OP states.

@rhaleblian
Copy link
Contributor

I removed the chmods in
#119
I think i got them all...

@rhaleblian
Copy link
Contributor

Uhoh, changing PREFIX reveals bugs. Will fix.

@rhaleblian
Copy link
Contributor

Ok.

ray@bronxcheer:~/Developer/retropie/linapple $ PREFIX=/tmp/local make install
install -d "/tmp/local/bin"
install build/bin/linapple "/tmp/local/bin/linapple"
install -d "/tmp/local/share/linapple"
install build/share/linapple/* "/tmp/local/share/linapple"
ray@bronxcheer:~/Developer/retropie/linapple $ tree /tmp/local
/tmp/local
├── bin
│   └── linapple
└── share
    └── linapple
        ├── linapple.conf
        └── Master.dsk

3 directories, 3 files

rhaleblian added a commit to plateofshrimp/linapple that referenced this issue May 18, 2020
@rhaleblian
Copy link
Contributor

rhaleblian commented May 18, 2020

then they use this separate install directory to wrap up the installer package (Red Hat "rpm", Debian "deb", etc) - which is not how the current "make package" works.

That's almost there. Should open a new issue for it.

@0cjs
Copy link
Contributor Author

0cjs commented May 19, 2020

└── share
└── linapple
├── linapple.conf
└── Master.dsk

@rhaleblian As I mentioned in the message for PR #125's commit, I think we want to have (in the source) and be distributing only a linapple.conf.sample, as the sensible default configuration is "empty or no config file." (Because linapple has internal configuration defaults, we should not have a second set of defaults read from a file overriding those because DRY.)

That said, I don't want to get in the way of or add confusion to your current work, so I'm just mentioning this here to keep in mind if you like. Once you've done this current bit of cleanup/improvement I'm happy to look in more detail into how we should handle the distribute/sample config file(s).

@rhaleblian
Copy link
Contributor

Sounds good. Let's stay DRY!

@rhaleblian
Copy link
Contributor

BTW I claim that the PR is ready for the big time. Stakeholders may want to test-drive it to assure they like the workflow and it doesn't break any workflow they need.

@maxolasersquad
Copy link
Member

@0cjs Should we instead ship a linapple.conf.sample with the defined defaults and remove the predefined defaults in the code? I have no strong argument for this other than it seems like that is how most software ships.

@maxolasersquad
Copy link
Member

Another option is that we have defaults defined as constants and then we produce linapple.conf.sample file from those defaults and also use those constants when no configuration is given. That keeps everything dry

@rhaleblian
Copy link
Contributor

rhaleblian commented May 21, 2020

Hmm, yes, there are different ways to stay DRY.

One pro of demanding a file is that it provides users with a way of seeing default applicaton settings which they would otherwise not be able to see (docs or Github nonwithstanding).

That's a pretty big pro, thinking about it...

If users can self-answer "the file goes where?" and "oh it already does what i want" questions, they might find they don't need to customize at all, or minimally. ["look Mommy, why are those developers over there smiling?"]

@0cjs
Copy link
Contributor Author

0cjs commented May 21, 2020

@maxolasersquad That's certainly an option, but has its downsides, too, such as linapple failing to start if it can't find a config file, or an old config file that's missing certain settings is being used. Possibly going the other way, where the program can generate the default config file, is a better way to go, though of course that may introduce complexities of its own that need to be considered. (Either way, I agree with @rhaleblian that users being able to see default settings is quite important.)

I'm probably going to have a look at this in the next month or two, but don't let that stop anybody else from starting an issue or PR on it. (If you can mention it in this thread, or "@" me in the new one, that would be great.)

@rhaleblian
Copy link
Contributor

For now let's install the file with additional '.sample' extension and line it up with the configuration.

@rhaleblian
Copy link
Contributor

Any more convo re the conf file and mechanism would be best continued in a new issue.

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

No branches or pull requests

4 participants