jcnelson / vdev

A device-file manager for *nix
GNU General Public License v3.0
101 stars 13 forks source link

[Makefile] Separate DESTDIR from the other VARIABLES #40

Open tokiclover opened 9 years ago

tokiclover commented 9 years ago

Please separate DESTDIR variable from the other VARIABLES because it is the way do this kind of things; otherwise, it become quickqly cumbersome to modify any with prepending DESTDIR before system directory. See sys-libs/libpstat/files/Makefile-DESTDIR.path and or sys-fs/fskit/files/Makefile-DESTDIR.path or simply supervision as a sipme examle (which can be found in ubiquitous packages in the intertubes.)

Second, /usr should not be DESTDIR but only PREFIX! This one is very important because you should not confuse them. -- DESTDIR is usually not defined in 100% of makefile out there; and PREFIX has usually /usr/local value instead of simply /usr for compliancy, historic and manual installation reasons.

Thanks.

NOTE: Yes I am trying to make Gentoo packages!

tokiclover commented 9 years ago

Sorry to have forgotten the links; fixed now.

tokiclover commented 9 years ago

Of course this issue is valid for fskit and libpstat. Also forgotten in the first comment.

jcnelson commented 9 years ago

Confirmed; will take a look as soon as I can.

tokiclover commented 9 years ago

To finish succintely, I can look at autotools/automake to make a configure.ac and Makefile.{am,in} and resolve many things with one shot because I do not have the time to grasp the internal of vdev to get what is necessary to patch the Makefile... Though this will require autotools/automake dependencies for the git tree; and generating Makefile(s) and configure script can be done for futur versioned tarballs. This will simplify or externalize too much handling of sub-dirs and sub-projects dependencies at hand which would be taken care with the power of autotools/automake.

What do you think about it? I could open an new issue later in case of a positive answer.

jcnelson commented 9 years ago

@tokiclover Feel free to do so downstream, but please be informed that I do not have the time or energy to maintain support for GNU autotools, nor do I believe they are necessary for vdev. I will not be accepting a pull request for adding support for GNU autotools to the build system in the foreseeable future.

Recall that the primary goal of GNU autotools is to provide a consistent build environment on wide variety of operating systems with minimal source modifications. It is my current judgement that this is unnecessary for vdev, since vdev is only going to support running on Linux (no less than 2.6.24) and later on OpenBSD (once the Linux port is stable). The added costs of maintaining support for a complex build system exceed the benefits it would yield.

I do not have the time to grasp the internal of vdev to get what is necessary to patch the Makefile

If you intend to add an alternative build system, you might want to take the time to understand vdev's current build system and code layout first :). I'm happy to answer any questions you might have on that matter.

tokiclover commented 9 years ago

Come on @jcnelson... no need to write all that stuff instead of simply saying no to simple suggestion which would avoid wasting unnecessary time on both side. You should accept or refuse suggestions quite simply in a straight forward manner along with dealing with issues.

Well, my bad, lesson learned (from my side regarding the treatment of this suggestion and other issues.) I won't bother openning issues which aren't critical nor suggest anything beyond the immediat scope of vdev.

I wish you the best and hope other people would contribute to this project in order to get a worthy udev alternative. Good luck!

jcnelson commented 9 years ago

I wish you the best and hope other people would contribute to this project in order to get a worthy udev alternative. Good luck!

Thanks!

jcnelson commented 9 years ago

(accidentally hit "close and comment")

jcnelson commented 9 years ago

Okay, I think this is fixed as of ffc1f0625aa3cb3b091948df10ab54209d2769ee.

tokiclover commented 9 years ago

Not quite... to be polite. You're forcing to define DESTDIR and then, one has to include the value of DESTDIR every time when appending another variable but PREFIX. PREFIX value should not include the value of DESTDIR, nor LIBDIR, ETCDIR (which is usually and commonly named CONFDIR instead) et al. Each variable has a specific meaning that are not intertwined together to make sense of defining theme in the first place.

Think about this. If you keep tangling those variables together; defining and generating vdev.pc (or/and libudev-compat.pc) will not be possible if one is needed in the future which is, by the way, likely to happen because of the presence of... libudev-compat. How are you going to separate the value of DESTDIR from LIBDIR et al once tied together? Are you going to include the value DESTDIR in the .pc file? Nonsense.

You might look at the Makefile from LZ4 which makes use of only a makefile. And the .pc file section is one of interest as well.

jcnelson commented 9 years ago

See c97e20cb4e97ac1f8d2202eb34e2a627eafc7bbb and 8317212ef5549f6da85bda299ad8f6469c0627cb.

tokiclover commented 9 years ago

It appears... you're making funny commits by making cosmetic commits instead of... addressing the issue at hand. My bad... I did not know you will be reacting and doing this kind of cinema instead of accepting simple pointers, criticisms and issues kindly opened by users in order to improve this project. So what are expecting with this kind of bad joke?!

Ciao! I might never came here again depending on your response/actions in response to this. I don't want to waste my time looking at those kind commit just to kill some times. -- I do not need this kind of entertainment, no thanks.

NOTE: I don't know if I shall laugh or not... Yellow laugh if you ask me. LULZ.

jcnelson commented 9 years ago

@tokiclover I'm going by the documentation here [1]. Your issue was specifically with DESTDIR being separate from other variables, and I believe that my last two commits have made buildconf.mk consistent with sections 7.2.4 and 7.2.5. Is this not the case?

[1] https://www.gnu.org/prep/standards/html_node/Makefile-Conventions.html

jvvv commented 9 years ago

I'm not sure if this is what toki is referring to, but I did find that in buildconf.mk, USRSBINDIR and USRSHAREDIR are not defined. I looked through all of the makefiles looking for places where the install target might (accidently) install outside of DESTDIR if it's defined. The aforementioned undefined variables are all I spotted. While your makefile syntax usage is different than what I've seen in many autotooled or GNU sources (this isn't a bad thing, AFAIAC), they looked effective to my eyes. Once the USRSBINDIR and USRSHAREDIR are defined, then I think install targets should place everything into $DESTDIR, as expected.

tokiclover commented 9 years ago

On Fri, Aug 28, 2015 at 02:52:20PM -0700, Jude Nelson wrote:

@tokiclover I'm going by the documentation here [1]. Your issue was specifically with DESTDIR being separate from other variables, and I believe that my last two commits have made buildconf.mk consistent with sections 7.2.4 and 7.2.5. Is this not the case?

[1] https://www.gnu.org/prep/standards/html_node/Makefile-Conventions.html

Yes, precisely! There is practicaly nothing changed, again, but the removal of DESTDIR value from PREFIX.

And if you look at [1] ref. in the 7.2.4 DESTDIR sub-section, one can see this kind of examles:

$(INSTALL_PROGRAM) foo $(DESTDIR)$(bindir)/foo
$(INSTALL_DATA) libfoo.a $(DESTDIR)$(libdir)/libfoo.a

If you look at it, one can pretty much see that prefix/PREFIX and and libdir/LIBDIR are separated from those two directory variables, and thus, are used in the installation rules contrary to your incorrect usage (in buildconf.mk):

PREFIX         ?= /usr/local
INCLUDE_PREFIX ?= $(PREFIX)
BINDIR         ?= $(DESTDIR)$(PREFIX)/bin
SBINDIR         ?= $(DESTDIR)$(PREFIX)/sbin
LIBDIR         ?= $(DESTDIR)$(PREFIX)/lib
INCLUDEDIR     ?= $(DESTDIR)$(INCLUDE_PREFIX)/include
ETCDIR          ?= $(DESTDIR)$(PREFIX)/etc

incorrect usage of DESTDIR in the definition of other directory variables.

What do you want me to say when your link confirm my previous remarks and Makefile still define the directory variables with the value of DESTDIR after that many commits?

I've lost my Latin already after putting down so many words down just to get ignored again and again.

fbt commented 9 years ago

You're being antagonistic for no good reason. There is a misunderstanding here, and if you want it to be fixed, explain it without being an ass.

On the actual topic: yes, using DESTDIR in anything but install targets is wrong and makes life for packagers harder by being basically useless.

I personally just was too lazy to go through the makefiles to fix it, as they are pretty complex. I'm sure Jude would be perfectly fine with accepting a pull request that fixes his makefiles. Am I wrong?

fbt commented 9 years ago

I was going to leave this bit till vdev is almost ready to have a proper release, as it's not that important on the alpha stage, but let me explain DESTDIR from the point of view of a packager:

DESTDIR is the directory that would be root (/) on target systems. Which means that it must only be prepended to install targets and NEVER touch anything else inside the root. So, for example, the config makefile ends up getting DESTDIR into vdevd.conf, which is obviously wrong, and the current makefiles don't offer me an obvious way to override this without also breaking the packaging process:

acls=/home/fbt/git/pkg/vdev-git/pkg/vdev-git/usr/etc/vdev/acls
actions=/home/fbt/git/pkg/vdev-git/pkg/vdev-git/usr/etc/vdev/actions
ifnames=/home/fbt/git/pkg/vdev-git/pkg/vdev-git/usr/etc/vdev/ifnames.conf

Think of it this way: you want DESTDIR to not exist on the target machine the package is being built for. It's just a dir we put files into to make a snapshot from it. It must not interfere with anything else.

fbt commented 9 years ago

I've created a pull request with a partial fix to buildconf.mk.

jcnelson commented 9 years ago

@fbt: Thank you for your helpful explanation regarding DESTDIR, and thank you for your pull request. This makes a lot more sense to me, and I can see now why the buildconf.mk was incorrectly constructed. I have pushed some more patches to the build system that hopefully fix the remaining problems with libudev-compat and the config files. In particular, DESTDIR is no longer used to construct the fields in the config file.

@tokiclover: With @fbt's pull request, DESTDIR is now used to construct the INSTALL_* variables in the buildconf.mk file, which are then used to construct install targets in each Makefile. The effect should be the same as including DESTDIR in each install target directly.

@jvvv: I don't think USRSBINDIR is used anywhere anymore, but USRSHAREDIR is. Thank you for bringing it to my attention; I've just pushed a patch that should correctly define it.

jcnelson commented 9 years ago

Keeping this issue open, until we're sure that all the Makefiles correctly honor DESTDIR. Thank you all for your patience.

fbt commented 9 years ago

So far so good. I've changed my PKGBUILD, it's now much easier to work with. One thing: ETCDIR_VDEV could default to $(ETCDIR)/vdev

jcnelson commented 9 years ago

@fbt Just tweaked buildconf.mk to do so.

fbt commented 9 years ago

Sweet, thx!