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

Remove arch and zfs version dependency #8914

Merged
merged 8 commits into from
Jun 22, 2019
Merged

Remove arch and zfs version dependency #8914

merged 8 commits into from
Jun 22, 2019

Conversation

gordan-bobic
Copy link
Contributor

@gordan-bobic gordan-bobic commented Jun 16, 2019

Remove arch and zfs version dependency

Signed-off-by: Gordan Bobic gordan@redsleeve.org

Motivation and Context

zfs-dracut is automatically built for whatever architecture zfs is being compiled for. This is incorrect since zfs-dracut only contains bash scripts and is thus compatible with any arch.

Additionally, version dependency is unnecessary since this only requires a set of functionality basic enough to import the rootfs. For example, zfs-dracut 0.7.x works absolutely fine for creating bootable initramfs using zfs 0.8.x binaries.

It doesn't fix this issue, but it does help produce a zfs-dracut rpm that can be installed without errors and overrides when combating a regression situation:
#8913

Description

It's a relatively trivial change - it makes zfs-dracut build as a noarch package since it only contains bash scripts. It also removes the package's dependency on zfs of a particular arch and version since the functionality it needs is minimal.

@codecov
Copy link

codecov bot commented Jun 17, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8914      +/-   ##
==========================================
+ Coverage   78.66%   78.75%   +0.08%     
==========================================
  Files         382      382              
  Lines      117840   117840              
==========================================
+ Hits        92698    92804     +106     
+ Misses      25142    25036     -106
Flag Coverage Δ
#kernel 79.28% <ø> (-0.01%) ⬇️
#user 67.23% <ø> (+0.17%) ⬆️

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 c1b5801...4248f99. Read the comment docs.

@tonyhutter
Copy link
Contributor

  1. You have a good write-up of the motivation behind this PR. Can you include that text in your commit message? Also, can you add a Signed-off-by line?

Additionally, version dependency is unnecessary since this only requires a set of functionality basic
enough to import the rootfs. For example, zfs-dracut 0.7.x works absolutely fine for creating bootable
initramfs using zfs 0.8.x binaries.

Unfortunately, I think it does need to be dependent on the zfs version, as the modules in
contrib/dracut/90zfs/module-setup.sh.in may change. For example an 0.7.x zfs-dracut is not going to load the 0.8.x icp.ko module.

@gordan-bobic
Copy link
Contributor Author

gordan-bobic commented Jun 17, 2019

Apologies if this is a n00b question, is "signed-off" supposed to be by me or a reviewer?

Interesting, I hadn't expected 0.7.x to infer icp.ko dependency but 0.8.x to have it explicitly listed. Is there a reason why this has become explicit in 0.8.x?

I have re-added the version dependency without the release/arch parts.

@tonyhutter
Copy link
Contributor

Signed-off-by is for you (the author). The reviewers will automatically be tagged with a Reviewed-by line:

    Fix typo in vdev_raidz_math.c
    
    Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
    Reviewed-by: George Melikov <mail@gmelikov.ru>
    Signed-off-by: Brad Forschinger <github@bnjf.id.au>
    Closes #8875
    Closes #8880

Interesting, I hadn't expected 0.7.x to infer icp.ko dependency but 0.8.x to have it explicitly listed. Is
there a reason why this has become explicit in 0.8.x?

It's because 0.8.0 officially supported encryption (icp.ko), and channel programs (zlua.ko). Also, I see that the 0.8.x module-setup.sh.in includes spl.ko, which is wrong, since it was removed in 0.8.0. I assume that will be removed soon.

@tonyhutter
Copy link
Contributor

Also, I see that the 0.8.x module-setup.sh.in includes spl.ko, which is wrong, since it was removed
in 0.8.0. I assume that will be removed soon.

Oops, disregard this! spl.ko was moved into the zfs repo for 0.8.x; it was not removed. It still needs to be inmodule-setup.sh.in

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 19, 2019
@behlendorf behlendorf requested a review from tonyhutter June 20, 2019 19:50
@tonyhutter
Copy link
Contributor

I think you need:

 Requires:       %{name} >= %{version}-%{release}

Also, can you squash these commits, and add a commit message and a Signed-off-by line to the actual commit (via git commit --amend)?

@gordan-bobic
Copy link
Contributor Author

gordan-bobic commented Jun 21, 2019

I don't think -%{release} is required as it seems highly improbable that a feature will be patched in in a build time package patch.

Commit message modified. I don't have squash permissions on this side.

@behlendorf
Copy link
Contributor

@gordan-bobic I agree, the %{release} is needed. But the version seems like a good idea to ensure any expected features exist. I can squash then when merging if you like. You can do it locally as well but you'll need to force update the PR.

@gordan-bobic
Copy link
Contributor Author

Squashed from my side and added release version into version dependency.

@behlendorf
Copy link
Contributor

@gordan-bobic thanks, and I'm sorry. I badly typo'd above. I meant to say "I agree, the %{release} is not needed.". Can you update it one last time!

@gordan-bobic
Copy link
Contributor Author

No problem, @behlendorf, pushed.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 22, 2019
@behlendorf behlendorf merged commit ca4e5a7 into openzfs:master Jun 22, 2019
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 13, 2019
Remove arch and relax version dependency for zfs-dracut
package.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gordan Bobic <gordan@redsleeve.org>
Issue openzfs#8913
Closes openzfs#8914
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 22, 2019
Remove arch and relax version dependency for zfs-dracut
package.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gordan Bobic <gordan@redsleeve.org>
Issue openzfs#8913
Closes openzfs#8914
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 23, 2019
Remove arch and relax version dependency for zfs-dracut
package.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gordan Bobic <gordan@redsleeve.org>
Issue openzfs#8913
Closes openzfs#8914
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 17, 2019
Remove arch and relax version dependency for zfs-dracut
package.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gordan Bobic <gordan@redsleeve.org>
Issue openzfs#8913
Closes openzfs#8914
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Remove arch and relax version dependency for zfs-dracut
package.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gordan Bobic <gordan@redsleeve.org>
Issue openzfs#8913
Closes openzfs#8914
tonyhutter pushed a commit that referenced this pull request Sep 26, 2019
Remove arch and relax version dependency for zfs-dracut
package.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gordan Bobic <gordan@redsleeve.org>
Issue #8913
Closes #8914
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants