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

Update build files #5411

Closed
wants to merge 1 commit into from
Closed

Update build files #5411

wants to merge 1 commit into from

Conversation

dpronin
Copy link

@dpronin dpronin commented Oct 13, 2018

Updated configure, several config-scripts/*.m4, several Makefiles

  • in config-scripts/cups-common.m4 'AR' variable is used from environment (required for object-files compiled with -flto), in case it is not provided AR is set by searching for 'ar' program (by default as it was)
  • in config-scripts/cups-compiler.m4 'Os' imposed flag has been removed from OPTIM variable because it interferes with '-flto' if provided. 'Os', if required, can be improsed by CFLAGS/CXXFLAGS environment variables configurable by a user
  • in config-scripts/cups-opsys.m4 'CODE_SIGN' variable is set by searching for PATH to 'true' binary instead of using forcefully imposed '/usr/bin/true' because in some systems 'true' can be located in a different place. If 'true' cannot be found by this way the default behaviour is preserved and takes place (like it was)
  • in cups/Makefile, ppdc/Makefile, filter/Makefile, scheduler/Makefile, cgi-bin/Makefile 'LDFLAGS' is used when linking a shared library for being able to link at least with '-flto' flag. LDFLAGS should be provided always when linking, LDFLAGS as well as CFLAGS and CXXFLAGS can be set by a user whom configures a project
  • 'configure' script updated with 'autoconf --force > configure', version 2.69 of autoconf is used

Updated configure, several config-scripts/*.m4, several Makefiles

- in config-scripts/cups-common.m4 'AR' variable is used from environment (required for object-files compiled with -flto), in case it is not provided AR is set by searching for 'ar' program (by default as it was)
- in config-scripts/cups-compiler.m4 'Os' imposed flag has been removed from OPTIM variable because it interferes with '-flto' if provided. 'Os', if required, can be improsed by CFLAGS/CXXFLAGS environment variables configurable by a user
- in config-scripts/cups-opsys.m4 'CODE_SIGN' variable is set by searching for PATH to 'true' binary instead of using forcefully imposed '/usr/bin/true' because in some systems 'true' can be located in a different place. If 'true' cannot be found by this way the default behaviour is preserved and takes place (like it was)
- in cups/Makefile, ppdc/Makefile, filter/Makefile, scheduler/Makefile, cgi-bin/Makefile 'LDFLAGS' is used when linking a shared library for being able to link at least with '-flto' flag. LDFLAGS should be provided always when linking, LDFLAGS as well as CFLAGS and CXXFLAGS can be set by a user whom configures a project
- 'configure' script updated with 'autoconf --force > configure', version 2.69 of autoconf is used
@dpronin
Copy link
Author

dpronin commented Oct 13, 2018

My system is gentoo linux

> uname -a
Linux dannftk-gentoo-host 4.18.10-gentoo-dannftk #1 SMP Mon Oct 1 12:54:09 MSK 2018 x86_64 Intel(R) Core(TM) i3-4005U CPU @ 1.70GHz GenuineIntel GNU/Linux

I checked configuring by means of the following command

AR=llvm-ar CC=clang CXX=clang++ CFLAGS="-O2 -pipe -march=native -mtune=generic -flto -std=gnu11" CXXFLAGS="-O2 -pipe -march=native -mtune=generic -flto -std=gnu++17 -stdlib=libc++" LDFLAGS="-flto -fuse-ld=gold" ./configure

then I ran the command to check building

make all

everything has been built successfully

Actually, I didn't touch any *.dylib and libtool specific targets in Makefile, for me to haven't been able to verify them properly in my environment, let it alone I don't have Mac to check at least *.dylib, let it be the next iteration

I could check libtool specific targets and improve them if you like me to do that. For this I should adjust my environment

@michaelrsweet
Copy link
Collaborator

Can’t accept this as-is, but will review in detail next week.

@dpronin
Copy link
Author

dpronin commented Oct 14, 2018

I will be waiting for your review up impatiently

@michaelrsweet michaelrsweet self-assigned this Oct 14, 2018
@michaelrsweet michaelrsweet added the enhancement New feature or request label Oct 14, 2018
@michaelrsweet michaelrsweet added this to the CUPS 2.2.x Updates milestone Oct 14, 2018
Copy link
Collaborator

@michaelrsweet michaelrsweet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will incorporate many of these changes, as noted in the comments.

@@ -34,13 +34,18 @@ CPPFLAGS="${CPPFLAGS:=}"
CXXFLAGS="${CXXFLAGS:=}"
LDFLAGS="${LDFLAGS:=}"

dnl Setting compiler environment
AR="${AR:=}"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Autoconf's generated configure script already allows the environment variable to override the configure check.

Copy link
Author

@dpronin dpronin Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to pass these environment variables in 'configure' script without my changes and 'ar' is obtained from the system by searching for 'ar' program using PATH:

AR=llvm-ar CC=clang CXX=clang++ CFLAGS=-flto CXXFLAGS=-flto LDFLAGS=-flto ./configure && make

at the end of the build I see

/usr/bin/ar crvs libcups.a adminutil.o array.o auth.o backchannel.o backend.o debug.o dest.o dest-job.o dest-localization.o dest-options.o dir.o encode.o file.o getdevices.o getifaddrs.o getputfile.o globals.o hash.o http.o http-addr.o http-addrlist.o http-support.o ipp.o ipp-support.o langprintf.o language.o md5.o md5passwd.o notify.o options.o ppd.o ppd-attr.o ppd-cache.o ppd-conflicts.o ppd-custom.o ppd-emit.o ppd-localize.o ppd-mark.o ppd-page.o ppd-util.o pwg-media.o request.o sidechannel.o snmp.o snprintf.o string.o tempfile.o thread.o tls.o transcode.o usersys.o util.o

here manual says, quote,

Macro: AC_PATH_PROG (variable, prog-to-check-for, [value-if-not-found], [path = ‘$PATH’])

Like AC_CHECK_PROG, but set variable to the absolute name of prog-to-check-for if found. The result of this test can be overridden by setting the variable variable. A positive result of this test is cached in the ac_cv_path_variable variable.

I think it does not mean that the result can be ignored in a favor of an environment variable with the same name, does it?

@@ -98,7 +98,7 @@ if test -n "$GCC"; then
if test -z "$OPTIM"; then
if test "x$with_optim" = x; then
# Default to optimize-for-size and debug
OPTIM="-Os -g"
OPTIM="-g"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default compiler options need to specify optimization. Specify --with-optim="-g" to force a debug-only build, or to pass any other optimization options.

Copy link
Author

@dpronin dpronin Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is '-Os' intended for in default situation? I mean, maybe, I don't like this optimization to be in my package built, there is no need in it
I have believed all the compiler's optimization flags should be provided with CFLAGS, CXXFLAGS and linker's optimization and other flags with LDFLAGS
I mean, when you force a user to use -Os or any other unexpected flags in default situation it can as well unexpectedly interfere with the flags provided by generic environment variables, those are accepted by convention I presume
I wouldn't like to appear rude or stubborn but here you introduce some tricky hidden behavior with optimization by size. I would understand it if you added up flags like '-Wall' or '-fno-rtti' or something like this, because these options would be related to only the application itself, its robustness and restrictions
But, in my modest opinion, I don't think it is a good idea to make a user to experience the result of imposed optimization flags -O alike, I don't see any point in it, because these flags are related to what the application will be as a result of a user's choice
I mean, if a user wants it to be non-optimized they choose -O0, slightly optimized they set -O1 and so on, everything is under control
Here you say: "look, I have a hidden default behavior with optimization by size ('what for?' one would ask). To cancel it provide something another via the parameter '--with-optim'", what I think is not intuitive understandable, isn't it?

Copy link
Author

@dpronin dpronin Oct 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe, I would understand if you specified -O2 by default, ok, it would be understandable, -O2 is also by convention, like a trade-off in optimization levels. But -Os by default looks weird. No offence given

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

take a look how it is done in cmake
it can be food for mind

if test "$host_os_name" = darwin; then
CODE_SIGN="/usr/bin/codesign"
else
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'll use:

AC_PATH_PROGS(CODE_SIGN, codesign true)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if none is found? I think we should explicitly fail the build

@michaelrsweet
Copy link
Collaborator

[master c8c5ec3] Search for codesign/true, use LDFLAGS for shared libraries (Issue #5411)

@michaelrsweet
Copy link
Collaborator

[branch-2.2 bef1d62] Search for codesign/true, use LDFLAGS for shared libraries (Issue #5411)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants