clbr / radeontop

GNU General Public License v3.0
808 stars 70 forks source link

build system fixes #2

Closed hasufell closed 12 years ago

hasufell commented 12 years ago

remove getver.sh and handle it inside the Makefile (also overwritable by user) set programs properly and allow user to overwrite with non-PATH stuff respect CFLAGS, CC and LDFLAGS (only add flags that are crucial for compilation) add option for -DENABLE_NLS=1 remove stripping if debug is disabled and handle it via "make strip" use LIBS instead of LDFLAGS to add libraries add ncurses pkg-config calls reorder the compiler/linker line properly

clbr commented 12 years ago

I'll pull the parts I agree with ASAP.

clbr commented 12 years ago

Oh and thanks! I hope that wasn't too harsh.

hasufell commented 12 years ago

"my ncurses did not come with a pkg-config file, that would fail the build here"

"moving getver.sh to make + echo seems needless churn"

"I want the sections flags there to make small binaries, that's why they're there in the first place ;)"

"some of that moving to variables is completely overboard, IMHO, like $(RM)"

clbr commented 12 years ago

"that is a bug in your distribution then, because upstream provides them"

"That also means I cannot use "make VERSION="something"""

"On top of that compilation fails if ".git" directory is not existent btw which applies for github-generated tarballs."

"I reccommend to set them in your system, not hardcode them in Makefile, otherwise every downstream gotta fix it."

rm and a2x are system-independent, so using the host versions in case of cross-compiling doesn't hurt.

Anyway, please check current master, if there still are things lacking, please rebase your fixes.

clbr commented 12 years ago

In current master there is no more dep on git, for example. There may be "proper" tarball releases in the future when things stabilize a bit.

hasufell commented 12 years ago

"On purpose, I don't want a plethora of custom version strings"

"They didn't use to, and I don't have the latest ncurses installed."

"No point in setting them system-wide, when I only want them on some binaries. "

"BTW, do you (/gentoo) also remove those flags from all other packages defaulting to them, for example busybox?"

"Already fixed"

hasufell commented 12 years ago

how about we do the following:

$(shell pkg-config --libs ncurses 2>/dev/null || echo "-lncurses")

that should work

clbr commented 12 years ago

That's what I did in commit 0c3f239394b yesterday?

hasufell commented 12 years ago

oh, completely missed that :D

hasufell commented 12 years ago

else CFLAGS += -s endif

Is a very weird way to force stripped binaries over the user and ignore anything he might have set via his system/environment. IMO it should not be set at all in a Makefile. Makefiles should provide a "strip" or a "install-strip" rule for that like autotools.

There is still no option for nls, simply ifdef nls CPPFLAGS += -DENABLE_NLS=1 endif

would work (or do the reverse thing if you want it default). Please describe all available options at the top of the Makefile, cause we don't have a configure file here.

hasufell commented 12 years ago

LDFLAGS += -Wl,-O1 -Wl,-gc-sections

ldflags are still hacked, please do

LDFLAGS ?= -Wl,-O1 -Wl,-gc-sections

which will respect the systems/env LDFLAGS if present.

clbr commented 12 years ago

nls option & option comments added.

On ldflags additions, I want this binary to be sectioned. As mentioned above, those flags are completely safe.

hasufell commented 12 years ago

-ffunction-sections and -fdata-sections should not be forced on the user

http://blog.flameeyes.eu/2008/01/today-how-to-identify-unused-exported-functions-and-variables

clbr commented 12 years ago

Yes, I have read that article before, and I disagree with its premise.

It advocates a ton if ifdefs instead, which in any configurable app would quickly become unmaintainable.

hasufell commented 12 years ago

would you mind if you use seperate variables for those CFLAGS_EF = -ffunction-sections -fdata-sections CFLAGS += $(CFLAGS_EF)

LDFLAGS_EF = -Wl,-O1 -Wl,-gc-sections LDFLAGS += $(LDFLAGS_EF)

this would allow me to overwrite it via make LDFLAG_EF="" CFLAGS_EF="" without patching/sedding which makes it easier to maintain.

clbr commented 12 years ago

Yeah, that'd work. I'd like to hear why you object to -O1 to the linker though?

It's documented to either have no effect or an improvement, and recommended to always apply unless in need of fast builds.

hasufell commented 12 years ago

LDFLAGS are handled globally in gentoo (the user can also apply different ones for single packages via environment settings) and other source distros I know of. That means they generally don't want any kind of flags that are not needed for the package in order to compile except they greatly enhance/speed up the compiled binaries/libraries and cannot be set globally (like the section stuff). The section thing here is debatable and I don't have a strong opinion on it either, but making it overwritable is nicer imo.

As for the O1 thing it should rather be:

CFLAGS_EF = -ffunction-sections -fdata-sections CFLAGS += $(CFLAGS_EF)

LDFLAGS ?= -Wl,-O1 LDFLAGS_EF = -Wl,-gc-sections LDFLAGS += $(LDFLAGS_EF)

this will set -Wl,-O1 if there are no LDFLAGS in the system/env, but add -Wl,-gc-sections unconditionally unless the user unsets them on the make-line

clbr commented 12 years ago

Added.

hasufell commented 12 years ago

Thank you!

The last thing that's bothering me is the handling of stripping, could you elaborate why you don't just add a "install-strip" rule for that? Otherwise this will mess up system-internal handling of binary stripping.

clbr commented 12 years ago

Simply a matter of dev convenience, unless I'm debugging, I want stripped.

Seems Debian does a debug build, perhaps Gentoo could too? Is there some separated debuginfo packages, etc complicating things?

hasufell commented 12 years ago

I don't understand, gentoo does not pre-build packages. The user handles his (debug) flags almost alone, because no upstream can anticipate what he actually wants. We only provide a debug useflag if there are actual in-source switches which change behavior of the program and even then cflags will be filtered. For dev convenience you could use gentoo where you can manage your flags properly without touching build systems.

But talking generally... not every user will expect stripped binaries if he does not use your debug option. That's just your workflow and shouldn't be forced on the user.

This will require a patch downstream. Autotools offers a "install-strip" rule as already mentioned which does the exact same thing without doing unexpected changes/additions to flags.

clbr commented 12 years ago

Heh @ the conversion attempt. I'm stable in my religion, thank you.

Sorry, dev convenience > user convenience, every time. What autotools does may be de-facto standard, but it's not necessarily the best way.

hasufell commented 12 years ago

there is also "packager convenience"... sometimes packagers get lazy because of trivial issues they don't want to bother with on every version bump and then forget to package something...

at least include a "nostrip" option:

https://gist.github.com/3560567

clbr commented 12 years ago

Perhaps you could enlighten me why you want such a binary in the first place?

The limited symbol list is not useful for debugging/backtraces.

hasufell commented 12 years ago

No, you didn't understand... as I already said most proper source distros handle these things on their own which means: a) stripping is handled system internally and is fine-grained, your Makefile breaks this control b) debug flags are handled by the user, not by some random flags the Makefile adds. There is more than one debug flag btw., but you don't offer the choice. c) gentoo offers a "split-debug" option which again requires Makefiles not to mess with such things, cause they should not at all.

As a sidenote: if you would have used autotools we could have saved quite some time.

clbr commented 12 years ago

If I used autotools it would have taken more time for me, plus I would have done a similar change there too (strip by default) anyway.

To be clear, I don't care how advanced and fine system Gentoo has - I don't use Gentoo. This thread is going nowhere, closing.

clbr commented 12 years ago

To clarify, this is bikeshedding over a policy decision.

I suggest to simply always use the debug option, not exposing that as an use flag. Since you strip separately, it should work out.

For more debug options (-ggdb etc), they can be added to normal CFLAGS as usual, gcc will do the right thing.

hasufell commented 12 years ago

This is not just gentoo related. Any source distro handling compiler flags (I was not talking about useflags) and stripping on their own (which is the only proper way, cause you cannot have everything in scope they do or need to do) will conflict with this.

Proper build systems already know that.

What you suggest is just insufficient and blocks radentop from being distributed (at least by me). Thanks for merging some of the proposed changes anyway.