Couple of compile fixes and DESTDIR support

[Thermi]

Hello,

I fixed a couple of issues I encountered while compiling pev, as well as added support for the DESTDIR parameter.

Regards,
Thermi

[jweyrich]

Thank you for taking the time to contribute. If you don't mind, I have a few notes and questions regarding your changes.

7978bb7 - Makefile: Put compiler parameters in front of variable declarations, so they take effect
You revert this change in fdcde0b. Combining these 2 commits results in no changes. You may interactively rebase your branch to delete these commits.

671c61e - Makefile: Add support for DESTDIR
We already support DESTDIR. The values you propose for prefix go against the GNU conventions recommendations. In fact, our current prefix seems to be wrong as well - I'm the one to blame. It should be /usr/local, as recommended.

3b588f6 - Makefile: Reordered variable declarations to be in a nicer order
2de314b - Makefile: Reordered one more item in the variable declarations
These changes are good. Thank you!

fdcde0b - Makefile: Fixed overrides for compiler parameters
This is reverting 7978bb7.

45ff574 - Makefile: Fixed duplicate installation of files
You remove $(DEST) from the $(INSTALL_PROGRAM), and add it back as $(bindir) in ae178e5. In our current HEAD DEST is already set to$(DESTDIR)$(bindir), so I believe there's no need to change it. Please, would you clarify the need for this change? Most importantly, are you seeing any problem related to this in the current development revision (HEAD)?

554339b - plugins Makefile: Fixed path to libpe
The current plugins don't depend on libpe directly. I believe we should just remove any references to it (e.g.,-I$(LIBPE)). And as you pointed out, that path is wrong. Nice catch! It becomes wrong because of cd $(PLUGINS_DIR) && $(MAKE) $@. Since $(LIBPE) has a reference to srcdir, which is . (current dir), it results in ./../lib/libpe - it should be ./../../lib/libpe. We could just avoid the trouble by removing all these references, unless someone already has a plugin that depend directly on libpe - I'm open to suggestions.

ae178e5 - Makefile: Fixed binaries not getting installed and changed bindir to /usr/bin/
I believe there's no need to change it. DEST is already set to $(DESTDIR)$(bindir).

a141faf - Makefile: Changed bindir to /usr/bin
This goes against the GNU conventions recommendations.

I'll cherry-pick the 2 commits that are currently ok, and we can try to fine tune others if you are seeing any problems that they presumably fix. If you disagree with any point I raised, please, don't hesistate in replying here.

Will try to do this during the weekend - can't guarantee though.

[Thermi]

Hello,

7978bb7 - Makefile
The problem I found is that you declare LIBPE below the compiler options, so using $(LIBPE) in the compiler options results in nothing being inserted into LDFLAGS or CFLAGS, because the variable is empty.
I propose using this commit, as the other one (fdcde0b) is wrong and was done erroneously.

fdcde0b
See 7978bb7

ae178e5
45ff574
671c61e
DESTDIR currently doesn't work right now.
Do this:

mkdir pev-git
cd pev-git/
git clone https://github.com/merces/pev.git
cd pev/
make
git submodule init
git submodule update
git make
make
mkdir ../install
make DESTDIR=pwd/../install install

You see, that it errors out like this (With another base path for you)
make[1]: Entering directory '/home/thermi/nfs2/Software/pev-git/pev/src'
mkdir: cannot create directory '/usr/share/pev': Permission denied
Makefile:153: recipe for target 'installdirs' failed
make[1]: *** [installdirs] Error 1
make[1]: Leaving directory '/home/thermi/nfs2/Software/pev-git/pev/src'
Makefile:9: recipe for target 'install' failed
make: *** [install] Error 2

Hence, you should use most if not all of the commits that relate to DESTDIR.
I'll make another commit to change the default DESTDIR to "/usr/local/".

554339b
I think it's okay to include duplicate "-L"s into LDFLAGS, if they are supposed to point to the same library. The problem is, that you have "-lpe" in LDFLAGS, so the linker will abort, because it can't find libpe.

I'm sorry for the sometimes weird commits. I think this is all to blame for me
trying to work this out at times I usually sleep.

Regards,
Thermi

[jweyrich]

You're right about DESTDIR not working correctly. I must haven't tested it in a while. I merged and pushed those 2 commits that were already ok. Thanks! I also attempted to fix the remaining problems related to DESTDIR in 45ca201. Please, would you mind pulling to a new local branch and test whether it's working or not?
I'll take a second look on the remaining commits later during the week.

[Thermi]

Hello,

I looked over the file and see that it can't work correctly.
The reason is, that you install the files into $(DESTDIR)$(bindir).
bindir is set to $(exec_prefix)/bin and exec_prefix is set to exec_prefix = $(prefix).
prefix is set to prefix = /usr/local.
So the complete path will be $(DESTDIR)/usr/local/bin
So if you define a DESTDIR, the script will attempt to install the files into /usr/local in folders under DESTDIR and not into DESTDIR directly, like this: $(DESTDIR)/bin, $(DESTDIR)/share etc.

Regards,
Thermi

[jweyrich]

Ok. I have some doubts about it, but I reverted my changes and merged your commit 671c61e. Please, would you take a look again?

[Thermi]

Yes, it's working correctly now.

[jweyrich]

@Thermi I believe the commit I reverted was correct though. What we did now doesn't seem to respect the GNU Coding Standards: DESTDIR. Would you take a look and tell me your opinion?

[Thermi]

Hello,

Not even the coreutils handle DESTDIR according to the document you linked.
In the end, on every Linux operating system, binaries go into /bin or /usr/bin, so they are moved into place in the right place during packaging to be extracted onto the root directory and basicly fall into those places. Implementing DESTDIR the way described in the document would mean, that the packagers had to write large scripts to move the files from $(DESTDIR)/usr/local/ to $(DESTDIR)/, so they fall into those places. Nobody does that, so nobody implements DESTDIR described in the document. Every packaging script I saw expected the makefile to copy the files into $(DESTDIR)/(bin|usr/bin|etc), not $(DESTDIR)/usr/local/(bin|usr/bin|etc).

Regards,
Thermi

[jweyrich]

What version of coreutils are you referring to?
I just downloaded coreutils-8.23 and confirmed it uses DESTDIR as recommended by the GNU conventions. The Debian New Maintainers' Guide also recommends it.

[Thermi]

Hello, yes you are right. If you only use DESTDIR and don't specify prefix, it works as advised by the guide. However, that isn't the behaviour people usually want, so they specify prefix=/usr/ and sysconfigdir=/etc. That makes "make install" copy the files into the desired directory structure.

[merces]

Thanks, @Thermi and @jweyrich for the good discussion. I really appreciate!