linappleii / linapple

Apple 2e emulator
GNU General Public License v2.0
137 stars 61 forks source link

Cannot `make install` as non-root user #112

Open 0cjs opened 4 years ago

0cjs commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

rhaleblian commented 4 years ago

I removed the chmods in https://github.com/linappleii/linapple/pull/119 I think i got them all...

rhaleblian commented 4 years ago

Uhoh, changing PREFIX reveals bugs. Will fix.

rhaleblian commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

└── 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 commented 4 years ago

Sounds good. Let's stay DRY!

rhaleblian commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

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

rhaleblian commented 4 years ago

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