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.

Sign up for GitHub

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

Jump to bottom

Build fixes #10421

Closed
wants to merge 2 commits into from
Closed

Build fixes #10421

wants to merge 2 commits into from

Conversation

nivedita76
Copy link
Contributor

Motivation and Context

Currently when building ZFS as kernel built-in, there are references to the ZFS source directory, requiring it to be available when the kernel is compiled. This change fixes it to avoid such references and only use files that have been copied into the kernel source tree.

The second patch fixes an issue with VPATH builds for --with-config=user.

#10379

Description

How Has This Been Tested?

Tested builtin and modular builds.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@codecov
Copy link

codecov bot commented Jun 8, 2020 •
edited
Loading

Codecov Report

Merging #10421 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10421   +/-   ##
=======================================
  Coverage   79.54%   79.55%           
=======================================
  Files         391      391           
  Lines      123866   123866           
=======================================
+ Hits        98531    98540    +9     
+ Misses      25335    25326    -9
Flag Coverage Δ
#kernel 80.03% <ø> (+0.13%) ⬆️
#user 65.78% <ø> (+0.07%) ⬆️
Impacted Files Coverage Δ
lib/libzutil/zutil_pool.c 38.63% <0.00%> (-54.55%) ⬇️
cmd/zdb/zdb_il.c 30.86% <0.00%> (-24.08%) ⬇️
module/lua/lmem.c 83.33% <0.00%> (-4.17%) ⬇️
module/zcommon/zfs_fletcher.c 75.65% <0.00%> (-2.64%) ⬇️
module/zfs/zio_compress.c 89.74% <0.00%> (-2.57%) ⬇️
module/icp/algs/modes/gcm.c 79.25% <0.00%> (-2.56%) ⬇️
module/zfs/zil.c 91.25% <0.00%> (-1.87%) ⬇️
module/os/linux/zfs/zfs_file_os.c 84.15% <0.00%> (-1.00%) ⬇️
module/zfs/zfs_fm.c 84.39% <0.00%> (-0.87%) ⬇️
module/icp/api/kcf_cipher.c 15.76% <0.00%> (-0.83%) ⬇️
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7bcb7f0...d704d68. Read the comment docs.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 8, 2020
behlendorf
behlendorf requested changes Jun 8, 2020
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

This looks great. I put it through its paces locally and didn't uncover any problems. We'll want to rebase this and test it again once #10422 is merged and the kernel.org builder is passing. Thanks for cleaning this up.

module/Kbuild.in Show resolved Hide resolved
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 9, 2020
@nivedita76
Cleanup linux module kbuild files
a83d91f 
The linux module can be built either as an external module, or compiled
into the kernel, using copy-builtin. The source and build directories
are slightly different between the two cases, and currently, compiling
into the kernel still refers to some files from the configured ZFS
source tree, instead of the copies inside the kernel source tree. There
is also duplication between copy-builtin, which creates a Kbuild file to
build ZFS inside the kernel tree, and the top-level module/Makefile.in.

Fix this by moving the list of modules and the CFLAGS settings into a
new module/Kbuild.in, which will be used by the kernel kbuild
infrastructure, and using KBUILD_EXTMOD to distinguish the two cases
within the Makefiles, in order to choose appropriate include
directories etc.

Module CFLAGS setting is simplified by using subdir-ccflags-y (available
since 2.6.30) to set them in the top-level Kbuild instead of each
individual module. The disabling of -Wunused-but-set-variable is removed
from the lua and zfs modules. The variable that the Makefile uses is
actually not defined, so this has no effect; and the warning has long
been disabled by the kernel Makefile itself.

The target_cpu definition in module/{zfs,zcommon} is removed as it was
replaced by use of CONFIG_SPARC64 in
  commit 70835c5 ("Unify target_cpu handling")

os/linux/{spl,zfs} are removed from obj-m, as they are not modules in
themselves, but are included by the Makefile in the spl and zfs module
directories. The vestigial Makefiles in os and os/linux are removed.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
@nivedita76
Fix VPATH builds for user config
d704d68 
cmd/zpool and lib/libzutil Makefile's use -I., which won't work with a
VPATH build. Replace it with -I$(srcdir) instead.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
@nivedita76
Copy link
Contributor Author

This looks great. I put it through its paces locally and didn't uncover any problems. We'll want to rebase this and test it again once #10422 is merged and the kernel.org builder is passing. Thanks for cleaning this up.

I rebased onto 7bcb7f0. Re the builders, if I look at the details link, they seem to show only the second patch but not the first. Is that just a display error or are they not building with the first one?

@behlendorf
Copy link
Contributor

Thanks. We compile test the top 5 commits in the stack, but you'll need to click on the check mark next to the commit hash above to see that build status. The top commit in the PR will be fully tested. Things look good, so once the bots are done with it this should be ready to merge.

behlendorf pushed a commit that referenced this pull request Jun 10, 2020
@nivedita76 @behlendorf
Fix VPATH builds for user config
66786f7 
cmd/zpool and lib/libzutil Makefile's use -I., which won't work with a
VPATH build. Replace it with -I$(srcdir) instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes #10379
Closes #10421
BrainSlayer pushed a commit to BrainSlayer/zfs that referenced this pull request Jun 10, 2020
@nivedita76 @BrainSlayer
Cleanup linux module kbuild files
87cff99 
The linux module can be built either as an external module, or compiled
into the kernel, using copy-builtin. The source and build directories
are slightly different between the two cases, and currently, compiling
into the kernel still refers to some files from the configured ZFS
source tree, instead of the copies inside the kernel source tree. There
is also duplication between copy-builtin, which creates a Kbuild file to
build ZFS inside the kernel tree, and the top-level module/Makefile.in.

Fix this by moving the list of modules and the CFLAGS settings into a
new module/Kbuild.in, which will be used by the kernel kbuild
infrastructure, and using KBUILD_EXTMOD to distinguish the two cases
within the Makefiles, in order to choose appropriate include
directories etc.

Module CFLAGS setting is simplified by using subdir-ccflags-y (available
since 2.6.30) to set them in the top-level Kbuild instead of each
individual module. The disabling of -Wunused-but-set-variable is removed
from the lua and zfs modules. The variable that the Makefile uses is
actually not defined, so this has no effect; and the warning has long
been disabled by the kernel Makefile itself.

The target_cpu definition in module/{zfs,zcommon} is removed as it was
replaced by use of CONFIG_SPARC64 in
  commit 70835c5 ("Unify target_cpu handling")

os/linux/{spl,zfs} are removed from obj-m, as they are not modules in
themselves, but are included by the Makefile in the spl and zfs module
directories. The vestigial Makefiles in os and os/linux are removed.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes openzfs#10379
Closes openzfs#10421
BrainSlayer pushed a commit to BrainSlayer/zfs that referenced this pull request Jun 10, 2020
@nivedita76 @BrainSlayer
Fix VPATH builds for user config
be27abf 
cmd/zpool and lib/libzutil Makefile's use -I., which won't work with a
VPATH build. Replace it with -I$(srcdir) instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes openzfs#10379
Closes openzfs#10421
lundman referenced this pull request in openzfsonosx/openzfs Jun 12, 2020
@nivedita76 @lundman
Cleanup linux module kbuild files
9215821 
The linux module can be built either as an external module, or compiled
into the kernel, using copy-builtin. The source and build directories
are slightly different between the two cases, and currently, compiling
into the kernel still refers to some files from the configured ZFS
source tree, instead of the copies inside the kernel source tree. There
is also duplication between copy-builtin, which creates a Kbuild file to
build ZFS inside the kernel tree, and the top-level module/Makefile.in.

Fix this by moving the list of modules and the CFLAGS settings into a
new module/Kbuild.in, which will be used by the kernel kbuild
infrastructure, and using KBUILD_EXTMOD to distinguish the two cases
within the Makefiles, in order to choose appropriate include
directories etc.

Module CFLAGS setting is simplified by using subdir-ccflags-y (available
since 2.6.30) to set them in the top-level Kbuild instead of each
individual module. The disabling of -Wunused-but-set-variable is removed
from the lua and zfs modules. The variable that the Makefile uses is
actually not defined, so this has no effect; and the warning has long
been disabled by the kernel Makefile itself.

The target_cpu definition in module/{zfs,zcommon} is removed as it was
replaced by use of CONFIG_SPARC64 in
  commit 70835c5 ("Unify target_cpu handling")

os/linux/{spl,zfs} are removed from obj-m, as they are not modules in
themselves, but are included by the Makefile in the spl and zfs module
directories. The vestigial Makefiles in os and os/linux are removed.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes #10379
Closes #10421
lundman referenced this pull request in openzfsonosx/openzfs Jun 12, 2020
@nivedita76 @lundman
Fix VPATH builds for user config
2e1fcec 
cmd/zpool and lib/libzutil Makefile's use -I., which won't work with a
VPATH build. Replace it with -I$(srcdir) instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes #10379
Closes #10421
@nivedita76 nivedita76 deleted the include-dir-fix branch June 18, 2020 20:21
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
@nivedita76 @jsai20
Cleanup linux module kbuild files
d2b4e31 
The linux module can be built either as an external module, or compiled
into the kernel, using copy-builtin. The source and build directories
are slightly different between the two cases, and currently, compiling
into the kernel still refers to some files from the configured ZFS
source tree, instead of the copies inside the kernel source tree. There
is also duplication between copy-builtin, which creates a Kbuild file to
build ZFS inside the kernel tree, and the top-level module/Makefile.in.

Fix this by moving the list of modules and the CFLAGS settings into a
new module/Kbuild.in, which will be used by the kernel kbuild
infrastructure, and using KBUILD_EXTMOD to distinguish the two cases
within the Makefiles, in order to choose appropriate include
directories etc.

Module CFLAGS setting is simplified by using subdir-ccflags-y (available
since 2.6.30) to set them in the top-level Kbuild instead of each
individual module. The disabling of -Wunused-but-set-variable is removed
from the lua and zfs modules. The variable that the Makefile uses is
actually not defined, so this has no effect; and the warning has long
been disabled by the kernel Makefile itself.

The target_cpu definition in module/{zfs,zcommon} is removed as it was
replaced by use of CONFIG_SPARC64 in
  commit 70835c5 ("Unify target_cpu handling")

os/linux/{spl,zfs} are removed from obj-m, as they are not modules in
themselves, but are included by the Makefile in the spl and zfs module
directories. The vestigial Makefiles in os and os/linux are removed.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes openzfs#10379
Closes openzfs#10421
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
@nivedita76 @jsai20
Fix VPATH builds for user config
4b68703 
cmd/zpool and lib/libzutil Makefile's use -I., which won't work with a
VPATH build. Replace it with -I$(srcdir) instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes openzfs#10379
Closes openzfs#10421
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
@nivedita76
Cleanup linux module kbuild files
b57f780 
The linux module can be built either as an external module, or compiled
into the kernel, using copy-builtin. The source and build directories
are slightly different between the two cases, and currently, compiling
into the kernel still refers to some files from the configured ZFS
source tree, instead of the copies inside the kernel source tree. There
is also duplication between copy-builtin, which creates a Kbuild file to
build ZFS inside the kernel tree, and the top-level module/Makefile.in.

Fix this by moving the list of modules and the CFLAGS settings into a
new module/Kbuild.in, which will be used by the kernel kbuild
infrastructure, and using KBUILD_EXTMOD to distinguish the two cases
within the Makefiles, in order to choose appropriate include
directories etc.

Module CFLAGS setting is simplified by using subdir-ccflags-y (available
since 2.6.30) to set them in the top-level Kbuild instead of each
individual module. The disabling of -Wunused-but-set-variable is removed
from the lua and zfs modules. The variable that the Makefile uses is
actually not defined, so this has no effect; and the warning has long
been disabled by the kernel Makefile itself.

The target_cpu definition in module/{zfs,zcommon} is removed as it was
replaced by use of CONFIG_SPARC64 in
  commit 70835c5 ("Unify target_cpu handling")

os/linux/{spl,zfs} are removed from obj-m, as they are not modules in
themselves, but are included by the Makefile in the spl and zfs module
directories. The vestigial Makefiles in os and os/linux are removed.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes openzfs#10379
Closes openzfs#10421
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
@nivedita76
Fix VPATH builds for user config
2975e80 
cmd/zpool and lib/libzutil Makefile's use -I., which won't work with a
VPATH build. Replace it with -I$(srcdir) instead.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
Closes openzfs#10379
Closes openzfs#10421
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@nivedita76 @behlendorf