Make the configure script more packaging-friendly #98

dridi · 2017-05-16T14:55:10Z

Hello,

Someone's trying to introduce uftrace on Fedora [1] and I'd like to see this move forward. One problem is that the configure script doesn't meet packaging expectations, which is fixed in 77dcd9c ca9d704. That should make downstream maintainers lives easier since they'd less likely need to manually update the packaging whenever a guideline changes.

I also polished the script a bit.

Dridi

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1398922

namhyung · 2017-05-17T00:13:41Z

Thank you for fixing this, all look good to me.

@thepaul @bkircher @ganadist Are you ok with this change?

thepaul · 2017-05-17T01:01:27Z

configure

@@ -44,8 +44,8 @@ while getopts ":ho:-:p" opt; do
 	    # process --long-options
 	    case "$OPTARG" in
                help)  usage ;;
-                *=*)   opt=${OPTARG%%=*}; val=${OPTARG#*=}
-                       eval "$opt=$val" ;;
+                *=*)   opt=${OPTARG%=*}; val=${OPTARG#*=}


Any particular reason for changing %% to %? The %% seems preferable, since it should be possible for option values to contain = themselves, i.e. --prefix=/my/weird=dir/. Maybe not likely to be needed unless more general flags are added down the road, but afaict it doesn't hurt to keep it.

Yeah, kind of a typo because I initially tried to do the s/-/_/ substitution here and forgot to restore it properly.

thepaul · 2017-05-17T01:08:05Z

Ok with the change other than my quibble about ${foo%%x} vs ${foo%x}. This won't affect the deb packaging much.

namhyung · 2017-05-17T01:28:14Z

configure

@@ -81,6 +81,9 @@ fi
 if echo ${etcdir} | grep -qs /usr; then
    etcdir=$(echo ${etcdir} | sed -e "s^/usr^^")
 fi
+if [ -n "$sysconfdir" ]; then
+    etcdir=$sysconfdir
+fi


Please add it to the usage as well.

OK. By the way you may want to revisit the etcdir fixup because /usr/local/etc is actually the correct place for locally installed programs.

http://man7.org/linux/man-pages/man7/hier.7.html

Ah I didn't know that. Thanks for the info.

dridi · 2017-05-17T08:07:09Z

Thanks for the review, I just pushed fixes accordingly.

bkircher · 2017-05-17T08:45:47Z

Hi all!

I was able to change this %build section [1]

%build
export CFLAGS="%{__global_cflags}"
export LDFLAGS="%{__global_ldflags}"
./configure --prefix=%{_prefix} --libdir=%{_libdir}/%{name}
%make_build

into this

%build
%configure
%make_build

with the above changes. Will make Fedora packaging gods smile I suppose.

Anyways, I get a package build error now that I haven't investigated yet (Can be totally unrelated to this PR.)

$ rpmbuild -ba ~/rpmbuild/SPECS/uftrace.spec 
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.oGK9tL
+ umask 022
+ cd /home/bkircher/rpmbuild/BUILD
+ cd /home/bkircher/rpmbuild/BUILD
+ rm -rf uftrace-0.6.2
+ /usr/bin/gzip -dc /home/bkircher/rpmbuild/SOURCES/uftrace-0.6.2.tar.gz
+ /usr/bin/tar -xof -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd uftrace-0.6.2
+ /usr/bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ exit 0
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.Tr9Ccc
+ umask 022
+ cd /home/bkircher/rpmbuild/BUILD
+ cd uftrace-0.6.2
+ CFLAGS='-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic'
+ export CFLAGS
+ CXXFLAGS='-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic'
+ export CXXFLAGS
+ FFLAGS='-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -I/usr/lib64/gfortran/modules'
+ export FFLAGS
+ FCFLAGS='-O2 -g -pipe -Wall -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -m64 -mtune=generic -I/usr/lib64/gfortran/modules'
+ export FCFLAGS
+ LDFLAGS='-Wl,-z,relro -specs=/usr/lib/rpm/redhat/redhat-hardened-ld'
+ export LDFLAGS
+ '[' 1 = 1 ']'
+++ dirname ./configure
++ find . -name config.guess -o -name config.sub
+ '[' 1 = 1 ']'
+ '[' x '!=' x-specs=/usr/lib/rpm/redhat/redhat-hardened-ld ']'
++ find . -name ltmain.sh
+ ./configure --build=x86_64-redhat-linux-gnu --host=x86_64-redhat-linux-gnu --program-prefix= --disable-dependency-tracking --prefix=/usr --exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin --sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include --libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var --sharedstatedir=/var/lib --mandir=/usr/share/man --infodir=/usr/share/info
+ /usr/bin/make -O -j2
<snip/>
  LINK     uftrace
/usr/bin/ld: /home/bkircher/rpmbuild/BUILD/uftrace-0.6.2/arch/x86_64/uftrace.o: relocation R_X86_64_32 against `.rodata.str1.1' can not be used when making a shared object; recompile with -fPIC
/home/bkircher/rpmbuild/BUILD/uftrace-0.6.2/arch/x86_64/uftrace.o: error adding symbols: Bad value
collect2: error: ld returned 1 exit status
Makefile:203: recipe for target '/home/bkircher/rpmbuild/BUILD/uftrace-0.6.2/uftrace' failed
make: *** [/home/bkircher/rpmbuild/BUILD/uftrace-0.6.2/uftrace] Error 1
error: Bad exit status from /var/tmp/rpm-tmp.Tr9Ccc (%build)

@dridi I am the someone who submitted the package spec for review. Thanks for working on this. (BTW, are you able to sponsor me with that review request? 😉)

[1] uftrace.spec@f9ee5c7

bkircher · 2017-05-17T08:51:33Z

Amend previous comment: FYI, I actually still need to pass --libdir=%{_libdir}/%{name} to %configure.

namhyung · 2017-05-17T09:02:49Z

@dridi It'd be better if you could fold the fixes into the original commits and force push them to overwrite the branch.

dridi · 2017-05-17T09:05:33Z

@bkircher unfortunately I'm not a sponsor but I will help you with the package. I have other things that we can keep to bugzilla since they're not relevant here (including the libdir thing).

@namhyung will do, including a /usr/local/etc fix.

bkircher · 2017-05-17T09:05:36Z

Changes in this PR are unrelated to above build error it seems. I get the build error in current master, too. I'll investigate later.

From me: 👍

namhyung · 2017-05-17T09:16:29Z

@bkircher Thanks for the confirmation, please make it a separete issue. Providing detailed build instructions with make V=1 will help the investigation from my side.

dridi · 2017-05-17T09:20:16Z

Rebased.

namhyung · 2017-05-17T09:21:33Z

@dridi I forgot to say that I'd like to add your Signed-off-by line into the commit logs. Please read doc/contributing.md and add your sign-off. Sorry for the inconvenience!

Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

For RPM or Deb packaging where a configure script is expected to be compatible with autotools, long options may contain additional dashes that shouldn't be evaluated as variable names. Instead dashes are turned into underscores. At the same time, protect the $val with 'simple quotes' as it is done for positional arguments to avoid accidentally running commands when values contain whitespaces. While this takes care of whitespaces, there is still the possibility to run into a 'single quote' in the value but this is less likely. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

On Fedora or Red Hat systems, flags may be passed as long options without a value. On Fedora, a %configure macro is available to uniformly set up configure-based projects but it eats up the prefix: ./configure [...] --disable-dependency-tracking --prefix=/usr [...] Since this notation is not even documented, the '--long-option=value' notation can be enforced. Long options without a value are silently ignored. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

The only fix needed for the $etcdir is when /usr is the prefix. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

namhyung · 2017-05-17T09:32:00Z

@dridi @thepaul @bkircher Thank you all, will merge! :)

dridi · 2017-05-17T09:33:30Z

Signed off, apologies on my side for not researching it.

I think you can make it more noticeable if you move it to the root of your repository and rename it to CONTRIBUTING.md. This way when someone opens an issue or a pull request, they get this message from github:

Before you submit an issue, please review the guidelines for this repository.

To see it in action:

https://github.com/varnishcache/varnish-cache/issues/new
https://github.com/jsdnxx/CONTRIBUTING.md/issues/new

namhyung · 2017-05-17T09:35:33Z

Looks useful. I'll take a look, thanks for the info!

thepaul reviewed May 17, 2017

View reviewed changes

namhyung reviewed May 17, 2017

View reviewed changes

dridi added 6 commits May 17, 2017 11:24

configure: Generate .config using a heredocument

ce3c2cd

Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

configure: Use default for loop

d0d90cf

Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

configure: Allow setting $etcdir via --sysconfdir

f5f0baf

Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

configure: Leave /usr/local/etc alone

e86b53f

The only fix needed for the $etcdir is when /usr is the prefix. Signed-off-by: Dridi Boukelmoune <dridi.boukelmoune@gmail.com>

namhyung merged commit 478d102 into namhyung:master May 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make the configure script more packaging-friendly #98

Make the configure script more packaging-friendly #98

dridi commented May 16, 2017

namhyung commented May 17, 2017 •

edited

thepaul May 17, 2017

dridi May 17, 2017

thepaul commented May 17, 2017

namhyung May 17, 2017

dridi May 17, 2017

namhyung May 17, 2017

dridi commented May 17, 2017

bkircher commented May 17, 2017

bkircher commented May 17, 2017

namhyung commented May 17, 2017

dridi commented May 17, 2017

bkircher commented May 17, 2017

namhyung commented May 17, 2017 •

edited

dridi commented May 17, 2017

namhyung commented May 17, 2017 •

edited

namhyung commented May 17, 2017

dridi commented May 17, 2017

namhyung commented May 17, 2017

Make the configure script more packaging-friendly #98

Make the configure script more packaging-friendly #98

Conversation

dridi commented May 16, 2017

namhyung commented May 17, 2017 • edited

thepaul May 17, 2017

Choose a reason for hiding this comment

dridi May 17, 2017

Choose a reason for hiding this comment

thepaul commented May 17, 2017

namhyung May 17, 2017

Choose a reason for hiding this comment

dridi May 17, 2017

Choose a reason for hiding this comment

namhyung May 17, 2017

Choose a reason for hiding this comment

dridi commented May 17, 2017

bkircher commented May 17, 2017

bkircher commented May 17, 2017

namhyung commented May 17, 2017

dridi commented May 17, 2017

bkircher commented May 17, 2017

namhyung commented May 17, 2017 • edited

dridi commented May 17, 2017

namhyung commented May 17, 2017 • edited

namhyung commented May 17, 2017

dridi commented May 17, 2017

namhyung commented May 17, 2017

namhyung commented May 17, 2017 •

edited

namhyung commented May 17, 2017 •

edited

namhyung commented May 17, 2017 •

edited