Misc build fixes and improvements #17

tpetazzoni · 2018-04-22T21:25:15Z

Hello,

Here is a patch series doing misc build fixes and improvements:

Adds the possibility to detect the readline library using pkg-config, which allows to properly handle static linking configurations.
Fix a number of autoreconf warnings
Fix a build warning related to the use of major() and minor() macros
Add a .gitignore file to cleanup the output of "git status"

Thanks!

pkg-config automatically handles static linking situations, where for example readline is linked against ncurses, and therefore -lncurses needs to be passed in addition to -lreadline. This proposal uses pkg-config when available. If pkg-config is not found, or readline is not found via pkg-config, we fallback to the existing AC_CHECK_LIB(). This AC_CHECK_LIB() test is modified to set READLINE_LIBS, like PKG_CHECK_MODULES() does. The Makefile.am consequently uses READLINE_LIBS instead of hardcoding -lreadline. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Fixes the following autoreconf warning: cdrwtool/Makefile.am:3: warning: source file '../mkudffs/mkudffs.c' is in a subdirectory, cdrwtool/Makefile.am:3: but option 'subdir-objects' is disabled automake: warning: possible forward-incompatibility. automake: At least a source file is in a subdirectory, but the 'subdir-objects' automake: automake option hasn't been enabled. For now, the corresponding output automake: object file(s) will be placed in the top-level directory. However, automake: this behaviour will change in future Automake versions: they will automake: unconditionally cause object files to be placed in the same subdirectory automake: of the corresponding sources. automake: You are advised to start using 'subdir-objects' option throughout your automake: project, to avoid future incompatibilities. cdrwtool/Makefile.am:3: warning: source file '../mkudffs/defaults.c' is in a subdirectory, cdrwtool/Makefile.am:3: but option 'subdir-objects' is disabled cdrwtool/Makefile.am:3: warning: source file '../mkudffs/file.c' is in a subdirectory, cdrwtool/Makefile.am:3: but option 'subdir-objects' is disabled cdrwtool/Makefile.am: installing 'config/depcomp' udflabel/Makefile.am:3: warning: source file '../udfinfo/readdisc.c' is in a subdirectory, udflabel/Makefile.am:3: but option 'subdir-objects' is disabled Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Address the following libtoolize warning: libtoolize: Consider adding 'AC_CONFIG_MACRO_DIRS([m4])' to configure.ac, libtoolize: and rerunning libtoolize and aclocal. libtoolize: Consider adding '-I m4' to ACLOCAL_AMFLAGS in Makefile.am. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Fixes the following build warning: main.c: In function ‘is_whole_disk’: main.c:170:13: warning: In the GNU C Library, "major" is defined by <sys/sysmacros.h>. For historical compatibility, it is currently defined by <sys/types.h> as well, but we plan to remove this soon. To use "major", include <sys/sysmacros.h> directly. If you did not intend to use a system-defined macro "major", you should undefine it after including <sys/types.h>. maj = major(st.st_rdev); ^~~~~~~~~~~~~ main.c:171:13: warning: In the GNU C Library, "minor" is defined by <sys/sysmacros.h>. For historical compatibility, it is currently defined by <sys/types.h> as well, but we plan to remove this soon. To use "minor", include <sys/sysmacros.h> directly. If you did not intend to use a system-defined macro "minor", you should undefine it after including <sys/types.h>. min = minor(st.st_rdev); ^~~~~~~~~~~~~ main.c: In function ‘is_removable_disk’: main.c:231:13: warning: In the GNU C Library, "major" is defined by <sys/sysmacros.h>. For historical compatibility, it is currently defined by <sys/types.h> as well, but we plan to remove this soon. To use "major", include <sys/sysmacros.h> directly. If you did not intend to use a system-defined macro "major", you should undefine it after including <sys/types.h>. if (snprintf(buf, sizeof(buf), "/sys/dev/block/%d:%d/removable", major(st.st_rdev), minor(st.st_rdev)) >= (int)sizeof(buf)) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ main.c:231:13: warning: In the GNU C Library, "minor" is defined by <sys/sysmacros.h>. For historical compatibility, it is currently defined by <sys/types.h> as well, but we plan to remove this soon. To use "minor", include <sys/sysmacros.h> directly. If you did not intend to use a system-defined macro "minor", you should undefine it after including <sys/types.h>. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

pali · 2018-04-22T21:59:48Z

configure.ac

@@ -3,7 +3,7 @@ AC_PREREQ([2.63])
 AC_INIT(udftools, 2.0, , , [https://github.com/pali/udftools/])
 AC_CONFIG_AUX_DIR(config)
 AM_CONFIG_HEADER(include/config.h:include/config.in)
-AM_INIT_AUTOMAKE
+AM_INIT_AUTOMAKE([subdir-objects])


This change breaks making in parallel (make -j), as it generates same object files (default.o, file.o, ...) from concurrent jobs.

I don't see why this break parallel build, I just tested, it works fine. subdir-objects just tells automake that object files are produced in subdirs. Could you show the build failure you're seeing ?

It is how autoconf/automake generates Makefiles. More Makefile.am files refers to source files outside of current directory. Without this change, object files (from sources files) are always generated into current directory. With this change, object files are put to same location where are source files. Therefore parallel build starts overwriting generated object files in different directories -- classic race condition.

I don't get it sorry. Tons of projects are using subdir-objects with parallel build. subdir-objects will for example prevent object files from overwriting itself if you have foo/foo.c and bar/foo.c. If you have such file and don't use subdir-objects, then foo.o and foo.o will both be produced in the main directory, while with subdir-objects, they wil be produced as foo/foo.o and bar/foo.o.

I don't get it sorry. Tons of projects are using subdir-objects with parallel build.

So if you are proposing some change to merge, I guess you understand what it is doing. But seems not. Argument that tons of project are using it, does not mean that it would work also with udftools in current state.

subdir-objects will for example prevent object files from overwriting itself if you have foo/foo.c and bar/foo.c.

I do not contradict this. But udftools automake files (Makefile.am) does not use this pattern.

Problem is that e.g. Makefile.am for cdrwtool uses files outside of cdrwtool directory tree. Same for udflabel.

When subdir-objects option is not used, then object files compiled from those source files are put into the current main directory as you wrote. Which is the correct behavior, as cdrwtool does not produce any files outside of the cdrwtool directory.

But when subdir-objects is used, then cdrwtool compiles sources files outside of cdrwtool directory. And therefore starts fighting with other Makefile.am files which generates same file in their own directories.

So here not usage of subdir-objects is correct way how to avoid above race which you already described.

Correct way of course would be to not compile source files outside of the current Makefile.am directory, but that needs fixing Makefile.am automake files, not autoconf file.

pali · 2018-04-22T22:02:19Z

mkudffs/main.c

@@ -41,6 +41,7 @@
 #include <limits.h>
 #include <dirent.h>
 #include <sys/ioctl.h>
+#include <sys/sysmacros.h>
 #include <linux/fs.h>
 #include <linux/fd.h>
 #include <sys/sysmacros.h>


Why you are including sys/sysmacros.h file twice?

I guess it's a rebase issue, I'll drop this commit.

pali · 2018-04-22T22:03:55Z

configure.ac

@@ -3,7 +3,8 @@ AC_PREREQ([2.63])
 AC_INIT(udftools, 2.0, , , [https://github.com/pali/udftools/])
 AC_CONFIG_AUX_DIR(config)
 AM_CONFIG_HEADER(include/config.h:include/config.in)
-AM_INIT_AUTOMAKE
+AM_INIT_AUTOMAKE([subdir-objects])
+AC_CONFIG_MACRO_DIRS([m4])


Also this change breaks compilation, see Travis.

Could you give me a link to the Travis failure so that I can have a look ? Thanks!

It is above gitthub "comment" area. Now I reopened pull request, so travis started testing it again and you would see result there.

pali · 2018-04-22T22:16:02Z

.gitignore

+udffsck/udffsck
+udfinfo/udfinfo
+udflabel/udflabel
+wrudf/wrudf


I'm not a big fan of gitignore, it just hide if development git directory is clean or not, forces to use git clean -x (ignore gitignore). Also specially if people add there their own files which do not exists on other computers... Anyway everybody can sets its own set of gitignore rules (e.g. via file info/exclude) locally.

OK, I guess that's a matter of taste, but fair enough, I can drop this from the patch series.

pali · 2018-05-31T11:32:47Z

Hi @tpetazzoni, are you going to fix above mentioned problems?

pali · 2018-09-03T07:59:51Z

Closing for inactivity.

pali · 2018-10-25T16:37:29Z

@tpetazzoni ping

pali · 2018-11-08T15:34:39Z

Closing for inactivity.

tpetazzoni added 7 commits April 22, 2018 23:22

.gitignore: ignore files generated by autogen.sh

2ba8270

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

.gitignore: ignore files generated by ./configure

98ceae8

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

.gitignore: ignore files generated by the build

210a45b

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

pali requested changes Apr 22, 2018

View reviewed changes

pali closed this Sep 3, 2018

pali reopened this Sep 3, 2018

pali closed this Nov 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc build fixes and improvements #17

Misc build fixes and improvements #17

tpetazzoni commented Apr 22, 2018

pali Apr 22, 2018

tpetazzoni Sep 3, 2018

pali Sep 3, 2018

tpetazzoni Sep 3, 2018

pali Sep 3, 2018

pali Sep 3, 2018

pali Apr 22, 2018

tpetazzoni Sep 3, 2018

pali Apr 22, 2018

tpetazzoni Sep 3, 2018

pali Sep 3, 2018

pali Apr 22, 2018

tpetazzoni Sep 3, 2018

pali commented May 31, 2018

pali commented Sep 3, 2018

pali commented Oct 25, 2018

pali commented Nov 8, 2018

Misc build fixes and improvements #17

Misc build fixes and improvements #17

Conversation

tpetazzoni commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pali commented May 31, 2018

pali commented Sep 3, 2018

pali commented Oct 25, 2018

pali commented Nov 8, 2018