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

Makefile: more general way to respect BINDIR #93

Closed
wants to merge 1 commit into from
Closed

Makefile: more general way to respect BINDIR #93

wants to merge 1 commit into from

Conversation

aude
Copy link
Contributor

@aude aude commented Jun 17, 2016

This is more ideal in my opinion.

Glad for comments. I'm not familiar with .in files. Is there an other more appropriate way to do this?

@digint
Copy link
Owner

digint commented Jun 18, 2016

I'm not familiar with this ".in" files concept here either. Is this supposed to be some recommended way of installing auxiliary files? Does the install command recognize this and magically rename the target file?

The idea of having a hardcoded path in btrbk.service is that you can simply copy the files from the project and use it (as long as you use the default paths of course). While this is very useful for the btrbk executable, I'm not sure if this is of any real value for systemd service scripts.

@moben
Copy link
Contributor

moben commented Jun 18, 2016

foo.in files are commonly used by build systems, e.g. autotools to signify that they are used to generate foo. It's useful because there is an obvious distinction between the generated file and the input. There the @BAR@ syntax is used to mark a variable that will be replaced.
The install command doesn't care, but one upside to doing it this way is that the .in file doesn't change during make install and the output file can be added to .gitignore, which means git won't see the file as changed if you call make install (and thus the sed)

Being able to run btrbk standalone is nice but I'd assume that anyone who wants the systemd files uses the Makefile to install those anyway (or a distro package)

@aude
Copy link
Contributor Author

aude commented Jun 19, 2016

I see your point of being able to just copy the files. It's a really neat and appreciated feature, keeps it dead simple.

The reaction came as a result of BINDIR being used many places, but not for the systemd service. I would argue it's more intuitive that it's used everywhere.
https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=btrbk#n17

Here is an example of such .in files: https://git.archlinux.org/pacman.git/tree/contrib
Again, I have not used them, and would generally not be in favor if it adds complexity.

@aude
Copy link
Contributor Author

aude commented Jun 27, 2016

What do you think about this one?
I kindof think that the best way to do it is to not accept my pull request, or to use .in files. If we don't have the knowledge to use .in files, let's stay with the current setup.

As a way to clarify, maybe documenting the "use default paths" behavior could be clarifying for people looking at the code?

@digint
Copy link
Owner

digint commented Jun 28, 2016

I think the clean (and correct) way would be to have a ".in" file with @BINDIR@, and change the Makefile to 1. copy to btrbk.service.tmp, 2. replace (sed) @BINDIR@ on tmpfile, 3. install it.

I will clean that up on next release.

Also I'm thinking of:

  1. move from /usr/sbin/ to /usr/bin/ as the default path for btrbk
  2. remove the hardcoded $PATH in the main btrbk executable.

because

  1. btrbk by itself does not need root, it's the btrfs-progs called from btrbk which need it, and future btrbk releases will probably get some kind of "backend tools" configuration, allowing to use e.g. my (still very experimental) separated-executable suid/setcap patches on btrfs-progs
  2. This simply causes too much trouble with package maintainers (btrbk: Prefix PATH instead of resetting it. #74 for example), and does not allow nifty PATH tricks.

I'm leaving this pull request open as a reminder (no much free time to work on btrbk at the moment, I'm afraid)

@aude
Copy link
Contributor Author

aude commented Jun 29, 2016

I support this approach.

I updated my pull request and created some support for .in files using only sed. If the commit can be simplified further, please do so.

I would love to pipe the file directly to install, like sed -e ... btrbk.service.in | install - DEST, but install - DEST is unfortunately not supported.

@digint
Copy link
Owner

digint commented Jul 4, 2016

Thanks, the patch looks clean and usable this way.
I will merge it for the next feature release of btrbk. (I plan to build a bugfix release soon, without this change)

@digint
Copy link
Owner

digint commented Jul 13, 2016

merged in: 16d73b4

@digint digint closed this Jul 13, 2016
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

Successfully merging this pull request may close these issues.

3 participants