[LU-576] Incorrect build dependencies listed for libcfsutil.a

• Type: Bug
• Resolution: Low Priority
• Priority: Minor
• Fix Version/s: Lustre 2.1.0
• Affects Version/s: Lustre 2.0.0
• Labels: llnl
• Environment: Lustre 2.0.66
• Severity: 3
• Assignee: Minh Diep
• Reporter: Prakash Surya (Inactive)

Description

The dependencies listed for the libcfsutil.a target are incorrect. The sources for that library are listed in libcfs/libcfs/autoMakefile.am as:

libcfsutil_a_SOURCES = nidstrings.c util/parser.c util/parser.h util/platform.h \
                       util/l_ioctl.c util/util.c

Expanding out the above relative pathnames for the referenced [parser|platform].h files gives the following files:

libcfs/libcfs/util/parser.h
libcfs/libcfs/util/platform.h

The problem is these files do not exist. My assumption is, the dependencies it is intending to use are the following files:

libcfs/include/libcfs/util/parser.h
libcfs/include/libcfs/util/platform.h

Even worse, the [parser|platform].c files do not reference [parser|platform].h directly. These files are included through the directive:

#include <libcfs/libcfsutil.h>

But a dependency on libcfsutil.h is not in the *_SOURCES variable.

I think the right thing to do in this situation would be to fix these path issues using an "absolute" path to the needed files, using a reference to the top of the source tree. For example:

libcfsutil_a_SOURCES = $(top_srcdir)/libcfs/libcfs/util/nidstrings.c \
                       $(top_srcdir)/libcfs/libcfs/util/parser.c \
                       $(top_srcdir)/libcfs/libcfs/util/l_ioctl.c \
                       $(top_srcdir)/libcfs/libcfs/util/util.c \
                       $(top_srcdir)/libcfs/include/libcfs/util/parser.h \
                       $(top_srcdir)/libcfs/include/libcfs/util/platform.h \
                       $(top_srcdir)/libcfs/include/libcfs/libcfsutil.h

Although, full pathnames are not used throughout the build system for Lustre, thus the inconsistency of using full pathnames here is not ideal.

The patch submitted for LU-455, http://review.whamcloud.com/#change,1092, "fixes" this issue simply by removing the [parser|platform].h dependencies from the list of *_SOURCES.

Comments

[Christopher Morrone (Inactive)]

Mostly I want to be clear that we think this is likely a common problem in the build system. I don't think there is a point to fixing just these two. If we fix it, we should figure out the correct solution and apply it everywhere that it is needed.

I see no hurry to fix this either. I think that making some of the bigger changes like moving ldiskfs and the lustre kernel patches out of the lustre tree would be excellent first steps to cleaning up the build system. Once those are done, it will be easier to start some of the finer cleanup like this.

I would not suggest using the full path when the files are in the same directory as the makefile, or in subdirectories below the make file. I would perhaps only use the $(top_builddir) form of the path when we need to walk back up the tree. I am not really particularly opposed to using "..", though.

[Peter Jones]

chris,

Adding you for your comments

Thanks

Peter

[Prakash Surya (Inactive)]

My issue with using the full paths is that doesn't look to be the way things are done anywhere else in the build system. Even though I think it's the right fix, without going through and making the change everywhere, it still feels like an ugly hack. I'm sure I could look through the tree to answer this, but do you know how this sort of issue is handled in the other subsystems? I imagine having a separate 'include' directory is a common idiom.

And I haven't tried using '..' but technically I think it would work just fine. That solution just doesn't sit well with me.. Is there a good argument for '..' over full paths?

Isn't the benefit of using relative paths the fact that you can move the top level directories around and things will still work? If '..' is used then you lock yourself into the current directory structure without explicitly spelling out the structure you expect by using the full paths.

I know Chris Morrone also wasn't too keen using '..' to find the header files as well, although I'm not sure what his reasoning on that was. Perhaps it would be a good idea to add him to the watch list and see what he says about this matter.

[Brian Murrell (Inactive)]

I don't particularly have an issue with selective use of the full path, however couldn't a relative path also be used?

i.e.

libcfsutil_a_SOURCES = nidstrings.c util/parser.c ../include/libcfs/util/parser.h ../include/libcfs/util/platform.h \
                       util/l_ioctl.c util/util.c

[Peter Jones]

ok, thanks for clarifying Prakash

[Prakash Surya (Inactive)]

Peter,

No, I definitely think the LU455 patch should still land.

This "fix" I mention in the LU455 patch isn't changing any of the current behaviour. From the looks of it, it has always been broken, it was just wasn't being reported when using the DIST_SOURCES automake variable. So what was "broken" before is still "broken" with the LU455 patch, we just can no longer rely on the build system to hide the error for us.

Patches to properly fix this ticket should be made separately. Although, the "right" fix that I mention is probably only possible with a total rehaul of the build system.

[Peter Jones]

Brian

Could you please comment?

Prakash

Does this mean that we should not land the patch from LU455?

Peter