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

Man pages installed in /usr/share/man #441

Closed
Vouivre opened this issue Jan 2, 2022 · 15 comments
Closed

Man pages installed in /usr/share/man #441

Vouivre opened this issue Jan 2, 2022 · 15 comments

Comments

@Vouivre
Copy link
Contributor

Vouivre commented Jan 2, 2022

I've tried to packaged the new version of ytfzf with man pages. But the installation
failed because the man pages are directly installed in /usr/share/man.

gzip -c docs/man/ytfzf.5 > docs/man/ytfzf.5.gz
install docs/man/ytfzf.1.gz /usr/share/man/man1
install: can't create '/usr/share/man/man1/ytfzf.1.gz': Permission denied

Probably you should also use DESTDIR and PREFIX in the Makefile.

@Euro20179
Copy link
Collaborator

I habe absolitely no idea what Im doing with makefiles, if you make a pull request or even just comment here with a new makefile ill merge it

@ionenwks
Copy link
Contributor

ionenwks commented Jan 2, 2022

If using a Makefile, imo typically better to avoid doing too much (no gzip, nor automagic with uname to detect paths, users and distros will pick/compress as needed), support DESTDIR and PREFIX as pointed, and have a way to override individual paths, e.g. BINDIR and MANDIR.

Although given ytfzf is very simple to install I just skipped the Makefile in the gentoo ebuild for now (in our case man pages are compressed using user chosen compression and pre-compression is an issue).
Edit: in my own shell bash scripts project I ended up using meson anyway 👀 but it'd be overkill here, I also had tests and other stuff to manage and it made my life easier.

@Vouivre
Copy link
Contributor Author

Vouivre commented Jan 2, 2022

I'll have a look how the Makefile can be fixed. For the compression of the man pages, I would also not compress them. In kisslinux, the man pages are not compressed. Perhaps @Euro20179 or @ionenwks could review my proposal, because I don't know very well the variables BINDIR and MANDIR.

@Euro20179
Copy link
Collaborator

Euro20179 commented Jan 2, 2022

I'll have a look how the Makefile can be fixed. For the compression of the man pages, I would also not compress them.

Tested it to make sure this works, and it does so fair enough.

Perhaps Euro20179 or ionenwks could review my proposal, because I don't know very well the variables BINDIR and MANDIR.

I can review it, but like i said, I am a noob with Makefile, so i think jonenwks should also review it.

@Vouivre
Copy link
Contributor Author

Vouivre commented Jan 3, 2022

Here is a version which works on my system:

PROG=ytfzf

UNAME := $(shell uname)
ifeq ($(UNAME), Darwin)
    PREFIX = /usr/local
endif
ifeq ($(UNAME), FreeBSD)
    PREFIX = /usr/local
endif
ifeq ($(UNAME), Linux)
    PREFIX = /usr
endif

BINDIR=${PREFIX}/bin
MANDIR=${PREFIX}/share/man

man:
	mkdir -p ${DESTDIR}${MANDIR}/man1
	mkdir -p ${DESTDIR}${MANDIR}/man5
	install docs/man/ytfzf.1 ${DESTDIR}${MANDIR}/man1
	install docs/man/ytfzf.5 ${DESTDIR}${MANDIR}/man5

install:
	chmod 755 $(PROG)
	mkdir -p ${DESTDIR}${BINDIR}
	install ${PROG} ${DESTDIR}${BINDIR}/${PROG}

uninstall:
	rm -f ${DESTDIR}${MANDIR}/man1/ytfzf.1
	rm -f ${DESTDIR}${MANDIR}/man1/ytfzf.5
	rm -f ${DESTDIR}${BINDIR}/${PROG}

.PHONY: install uninstall man

I didn't test the "uninstall" rule, because I uninstall my softwares with my package manager.
The following could be changed:

  • it seems there is no difference between parentheses and curly braces. So we should choose
    between $(PROG) and ${PROG}. At the moment there is a mix of parentheses and curly braces.

  • as suggested by @ionenwks we could skip the detection of the os and directly set

    PREFIX=${DESTDIR}/usr/local
    
  • in some makefiles DESTDIR is defined to be empty, I don't know if it's necessary.

When all is clarified I can send a PR.

@Euro20179
Copy link
Collaborator

Euro20179 commented Jan 3, 2022

it seems there is no difference between parentheses and curly braces

I like curly braces, cause it matches shell script.

as suggested by @ionenwks we could skip the detection of the os and directly set

This is probably a good idea especially because (to my knowledge) ifeq is gnu make only.

EDIT:
actually, it might be a good idea to keep the os check, because it's already installed in /usr/bin on linux, changing that would be annoying.

@Vouivre
Copy link
Contributor Author

Vouivre commented Jan 3, 2022

I like curly braces, cause it matches shell script.

Ok, I'll use only curly braces in the patch.

EDIT: actually, it might be a good idea to keep the os check, because it's already installed in /usr/bin on linux, changing that would be annoying.

Ok, I'll keep it.

What about setting DESTDIR to an empty variable ? You can see an example here:

https://github.com/leahneukirchen/nq/blob/master/Makefile

There is the line

DESTDIR=

After searching, I don't find any good argument. My assumption is in this comment:

leahneukirchen/mblaze#4 (comment)

In this PR, the line containing

DESTDIR=

is deleted. I assume the dev want to keep this line such that the user knows DESTDIR can be set, but per
default it's set to the empty string. I think most of the time DESTDIR is not set to an empty value like in the actual Makefile. Just tell me what you prefer.

@Euro20179
Copy link
Collaborator

Just tell me what you prefer.

Personally I don't know enough about makefile to make this decision, whatever you think is better, or if @ionenwks has an opinion. (I'm not sure semantically what $DESTDIR is supposed to represent.)

@Vouivre
Copy link
Contributor Author

Vouivre commented Jan 3, 2022

Personally I don't know enough about makefile to make this decision, whatever you think is better, or if @ionenwks has an opinion. (I'm not sure semantically what $DESTDIR is supposed to represent.)

At the moment DESTDIR is not defined, so probably it's the best to not defined it.

Basically, when a package is created, the files are not directly installed in the usual directories. For ytfzf it
would be

/usr/bin/ytfzf
/usr/share/man/man1/ytfzf.1
/usr/share/man/man5/ytfzf.5

All the files are installed in a temporary directory. In kisslinux it's in

/home/user/.cache/kiss/proc/pkg/ytfzf

That's not exactly right, but the details bring nothing here. So DESTDIR is

DESTDIR="/home/user/.cache/kiss/proc/pkg/ytfzf"

DESTDIR is the destination directory to create a package. As a package maintainer I have to set the path correctly.

Then all files will be copied in DESTDIR. So when the Makefile has been executed I have on my computer

/home/user/.cache/kiss/proc/pkg/ytfzf/usr/ytfzf
/home/user/.cache/kiss/proc/pkg/ytfzf/usr/share/man/man1/ytfzf.1
/home/user/.cache/kiss/proc/pkg/ytfzf/usr/share/man/man5/ytfzf.5

The job for the package manager is then basically to create an archive of the DESTDIR directory and to uncompress it under /. So the structure of DESTDIR is

/home/user/.cache/kiss/proc/pkg/ytfzf/usr/bin/ytfzf
/home/user/.cache/kiss/proc/pkg/ytfzf/usr/share/man/man1/ytfzf.1
/home/user/.cache/kiss/proc/pkg/ytfzf/usr/share/man/man5/ytfzf5

After the package manager uncompress under / we get

/usr/bin/ytfzf
/usr/share/man/man1/ytfzf.1
/usr/share/man/man5/ytfzf5

Just tell me if something is not clear. Tomorrow I will create a PR.

@ionenwks I let you add comments if necessary.

EDIT: I hope I have interpreted correctly your last remark. I'm not sure.

@Euro20179
Copy link
Collaborator

Euro20179 commented Jan 3, 2022

EDIT: I hope I have interpreted correctly your last remark. I'm not sure.

Basically I wasn't sure what DESTIDR was, you interpreted it correctly.

At the moment DESTDIR is not defined, so probably it's the best to not defined it.

Then we will do that

@eli-schwartz
Copy link

Use of a non-existent variable (such as DESTDIR) expands to an empty nothingness.

Defining the variable so that it exists, but is empty, also expands to an empty nothingness.

Adding a line that does nothing because someone else on the internet said "I like to declare my nothingness explicitly" seems silly. :p

The ifeq Linux stuff is wrong -- Linux convention is to install to /usr/local unless using a package manager, in which case package manager build recipes will invoke make with an overridden PREFIX. Removing this special case means that:

  • you do the same thing everywhere
  • you no longer break in highly surprising ways on, say, OpenBSD, because OpenBSD is neither Darwin, nor Linux, nor FreeBSD, and hence on OpenBSD, PREFIX="" i.e. the empty string, i.e. make install fails with an error message to the effect that mkdir -p requires an operand, but was not provided one. If that command had not failed, you would still end up installing the ytfzf script to /ytfzf which seems wrong.

Since you fix an OpenBSD (and a dragonfly bsd, and a haiku, and a hundred other OSes) bug by simply removing the use of ifeq entirely, you end up with a POSIX makefile.

@Euro20179
Copy link
Collaborator

That pretty much convinced me that we should remove the OS dependant thing, but I'm still worried that it'll not work for people because in $PATH /usr/bin overrides /usr/local/bin, so when they update, they'll also have to remove the script from /usr/bin, if there's a way to do this in the make file, that'd be cool, otherwise we need to tell people.

@eli-schwartz
Copy link

That seems quite odd, since I'm fairly sure any system should always have /usr/local/bin before /usr/bin, which means /usr/local/bin is searched first and hence overrides /usr/bin

This is a common convention for e.g. the local system administrator overriding and customizing a distro packaged binary by dropping a modified copy, or a wrapper script, in /usr/local/bin.

@Euro20179
Copy link
Collaborator

Oh, fair enough, I customized my path because I was bored one day, and I thought that /usr/bin was before /usr/local/bin, you seem to be right though.

Vouivre added a commit to Vouivre/ytfzf that referenced this issue Jan 4, 2022
@Vouivre
Copy link
Contributor Author

Vouivre commented Jan 4, 2022

I've done a PR to fix the issues as discussed here.

Euro20179 added a commit that referenced this issue Jan 4, 2022
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