abishekvashok / cmatrix

Terminal based "The Matrix" like implementation
GNU General Public License v3.0
4.02k stars 417 forks source link

Honor user preference for install directory #9

Closed shinenelson closed 7 years ago

shinenelson commented 7 years ago

Currently, make install fails if run without sudo privileges.

This is because make is unable to install to /usr/share/consolefonts. All other configurations honors the prefix parameter that used to configure the Makefile except consolefonts.

I'm not sure whether this is because consolefonts MUST be installed system-wide and cannot be installed user-specific; or because it just missed to honor the prefix parameter.

There are multiple ways to approach this issue (which I can detail below). I would like to open a pull request with the changes, but then, unfortunately, I'm not familiar with make.

  1. Honor prefix and install to $(prefix)/share/consolefonts/ (and the other directories in the install-data-local function).
  2. Skip the installation of consolefonts (with a warning, probably) if make install is executed without sudo privileges. It is not like cmatrix wouldn't work (except -l) at all without the consolefonts right?

Breaking installation due to a non-trivial error is not desirable, IMHO.

abishekvashok commented 7 years ago

I'm not sure whether this is because consolefonts MUST be installed system-wide and cannot be installed user-specific; or because it just missed to honor the prefix parameter.

Installing system wide is the recommended way, however I would have a check if it can be installed per user.

Skip the installation of consolefonts (with a warning, probably) if make install is executed without sudo privileges. It is not like cmatrix wouldn't work (except -l) at all without the consolefonts right?

Yes.

abishekvashok commented 7 years ago

I am sorry to inform that:

I'm not sure whether this is because consolefonts MUST be installed system-wide and cannot be installed user-specific; or because it just missed to honor the prefix parameter.

It's a MUST :) consolefonts must be installed system wide.

Would you like to skip installing the font? I could provide a configure parameter to turn of installing the font.

shinenelson commented 7 years ago

That's OK. I could live without consolefonts. Can you set up a configure parameter to skip the installation of consolefonts so that make install doesn't break with Permission Denied while installing to a custom directory?

Like I said earlier,

It is not like cmatrix wouldn't work (except -l) at all without the consolefonts right?

And -l breaks on a terminal anyway ( See Issues #1 and #7 ) It should be configurable, IMO.

abishekvashok commented 7 years ago

That's OK. I could live without consolefonts. Can you set up a configure parameter to skip the installation of consolefonts so that make install doesn't break with Permission Denied while installing to a custom directory?

Sure on the way.

And -l breaks on a terminal anyway

Yes unless you set it explicitly, cause it's meant for tty.

abishekvashok commented 7 years ago

@shinenelson here you go: https://github.com/abishekvashok/cmatrix/commit/6525abe5e8059fe6cf524a5e84a8fb50031dfbb4

shinenelson commented 7 years ago

I tried with ./configure --disable-fonts --prefix /custom/path/. But I got hit with the same error as before :

 Installing matrix fonts in /usr/share/consolefonts...
/usr/bin/install: cannot remove '/usr/share/consolefonts/matrix.fnt': Permission denied
/usr/bin/install: cannot remove '/usr/share/consolefonts/matrix.psf.gz': Permission denied
Makefile:838: recipe for target 'install-data-local' failed
make[1]: *** [install-data-local] Error 1

Am I doing something wrong here?

Also, just looking at the commit https://github.com/abishekvashok/cmatrix/commit/6525abe5e8059fe6cf524a5e84a8fb50031dfbb4, it seems that you set a variable to enable fonts, but a flag to disable fonts and everywhere else you're checking whether the fonts was enabled. Is this the right approach? Or is this just my ignorance about make files talking?

abishekvashok commented 7 years ago

I am looking into it. It was working for me after I removed the old files recursively. I think my approach is right. If you feel wrong please criticise :)

abishekvashok commented 7 years ago

Remember ./configure is dynamically generated from configure.ac

shinenelson commented 7 years ago

I think my approach is right. If you feel wrong please criticise

I do not have any experience working with make.

But I still hit the same error (even after [re]moving the files manually, make is trying to install the fonts now). According to you, should the install-data-local function be called if the --disable-fonts is enabled during ./configure?

For me, it is still getting called which is why the issue is still happening.

./configure is dynamically generated from configure.ac

I knew the Makefile was generated dynamically during ./configure, I didn't know configure itself was dynamically generated. (Aside : If that's the case, then why should you be checking in the configure file? Should you let it be generated dynamically. But then, I do not know how to generate the configure file dynamically.) I know that you cannot manually edit the Makefile (because it is dynamically generated). I was looking at that file to figure what was going wrong. So, if you can write a test in the install-data-local function that says to test the --disable-fonts flag and skip it if enabled, that should practically fix it.

shinenelson commented 7 years ago

OK. Just to be sure, I tried installing to my custom directory after [re]moving the entire /usr/share/consolefonts directory and it worked fine. It is probably the test conditions in the install-data-local function that are breaking the installation. My system had only /usr/share/consolefonts, so, only that directory created trouble. [re]moving it installed cmatrix with no issues whatsoever.

All make installs is the binary file in $PREFIX/bin/ and the man page in $PREFIX/share/man/man1/. All this complication is just due to consolefonts. :confused:

abishekvashok commented 7 years ago

Happy to hear it's working.

Aside : If that's the case, then why should you be checking in the configure file? Should you let it be generated dynamically. But then, I do not know how to generate the configure file dynamically

Automake recommends giving a configuration file along with it's source, so that user's can just run onfigure, make and [sudo] make install. You can generate it from configure.ac by running autoreconf.

All make installs is the binary file in $PREFIX/bin/ and the man page in $PREFIX/share/man/man1/. All this complication is just due to consolefonts. 😕

Yes!

So shall I close this now? Is'nt everything ok?

shinenelson commented 7 years ago

Happy to hear it's working.

Is'nt everything ok?

No, it is not.

But I still hit the same error (even after [re]moving the files manually, make is trying to install the fonts now). According to you, should the install-data-local function be called if the --disable-fonts is enabled during ./configure?

For me, it is still getting called which is why the issue is still happening.

abishekvashok commented 7 years ago

OK. Just to be sure, I tried installing to my custom directory after [re]moving the entire /usr/share/consolefonts directory and it worked fine

shinenelson commented 7 years ago

That is not supposed to be the default behavior, is it?

Some systems (confirmed on Ubuntu 16.04 LTS) might have other fonts installed in /usr/share/consolefonts. If you removed the entire directory, then ./configure would not configure consolefonts because it looks for only 3 existing directories for consolefonts.

The objective here is to avoid the use of elevated (sudo) privileges for installing cmatrix which is un-necessary IMO. (cleaning out /usr/share/consolefonts would require sudo privileges too; and that could cause harmful effects for other applications or applications using consoles).

I had cmatrix installed from the ubuntu main repos which had installed the matrix.fnt and matrix.psf.gz in /usr/share/consolefonts/. Upon removing the package via apt, the [un-]installer removed only those 2 files and left the rest of the directory intact which means that the rest of the directory is not related to the application itself. This should be the expected behavior while attempting to compile the program from source too.

shinenelson commented 7 years ago

Also, if the --disable-fonts is passed to configure, shouldn't it skip calling the install-data-local function altogether when invoking install-am?

I think this is something to do with the way make is configured. I took a look at the Makefile again. The install flow looks like this if I do make install :

I couldn't make any sense of the configure script, but I think passing the --disable-fonts flag didn't work as expected. Passing the --disable-fonts flag should've made install-data-am -> install-man without the install-data-local. Invoking install-data-local is what is causing this problem.

From my previous comments :

Also, just looking at the commit 6525abe, it seems that you set a variable to enable fonts, but a flag to disable fonts and everywhere else you're checking whether the fonts was enabled.

All make installs is the binary file in $PREFIX/bin/ and the man page in $PREFIX/share/man/man1/. All this complication is just due to consolefonts. :confused:

I tried invoking make install-exec-am && make install-man with the /usr/share/consolefonts directory as is and it worked as expected and installed cmatrix correctly to the right directories / paths which confirms my theory above it could be something wrong with the --disable-fonts flag and the way it is configured when it is passed to ./configure.

abishekvashok commented 7 years ago

@shinenelson thanks for your inputs :+1:

Please see the latest commit: https://github.com/abishekvashok/cmatrix/commit/c85f375bebf0a1f3b97f0a8e7da28d5249b1b85c

and report back whether it's fixes this issue or not, I am getting correct outputs after this commit. So, please ensure you get correct results.

./configure --prefix=~ --without-fonts
make
make install

Note: as you know --prefix can be any directory.

shinenelson commented 7 years ago

This works as expected with the --without-flag, but it doesn't work as desired in the opposite scenario.

install-data-local completes execution with no issues even if there are Permission Denied issues. That's not desired. It should exit with a non-zero exit code.

There are 2 other things I'd like to point out too :

  1. make prints out the complete function upon execution, even though it completes execution with no issues. I think that is because you need to prefix the outermost if condition with an @ (?)
  2. I still don't understand the logic for why you have a variable called with_fonts and then check for a flag called --without-fonts. Are these 2 related at all? This was the same case earlier with enable_fonts which you renamed to with_fonts this time. What I don't get is how is the negative check handled by make. I don't think the literal meaning of the code makes sense here. :confused: :thinking:
abishekvashok commented 7 years ago

Are these 2 related at all? This was the same case earlier with enable_fonts which you renamed to with_fonts this time. What I don't get is how is the negative check handled by make. I don't think the literal meaning of the code makes sense here.

It matters in the loop, with_fonts is used since we use the without-flag

make prints out the complete function upon execution, even though it completes execution with no issues. I think that is because you need to prefix the outermost if condition with an @ (?)

Oh, I think I missed that. I would get a fix in.

This works as expected with the --without-flag, but it doesn't work as desired in the opposite scenario.

As I mentioned earlier, console fonts needs to be installed in the /usr/ directory and not in any user defined prefix.

install-data-local completes execution with no issues even if there are Permission Denied issues. That's not desired. It should exit with a non-zero exit code.

You will get the error code. I am getting permission denied.

abishekvashok commented 7 years ago

@shinenelson are any more clarifications required as of now?

shinenelson commented 7 years ago

I am sorry for such a late response. I have been busy.

I did a round of testing today and found that the issues raised here have been mitigated well enough. Thanks @abishekvashok for your support in fixing this issue. We can close this issue now.