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

rpm,deb: remove conditional BuildRequires for btrfs-progs #8016

Merged
merged 1 commit into from Mar 21, 2016

Conversation

ErwanAliasr1
Copy link
Contributor

By pull request 7742, the btrfs-progs package was considered as a BuildRequires
only when --with tests was engaged like :

if %{with tests}
BuildRequires: btrfsprogs
%endif

That's perfectly valid for a spec file.

The issue we have is the following :

  • yum-builddep called by install-deps.sh is used to prepare the build env by
    installing the needed BuildRequires.
  • %{with test} is defined by using a %bcond_with
  • yum-builddep doesn't consider %{with test} as valid
  • yum-builddep doesn't install the btrfs package

As per discussions with the yum team, there is no way to engage conditional flags with
yum-builddep.

So this patch, as per discussions with Nathan Cutler & Loic Dachary, is removing
the condition arond the BuildRequires.

Note that all BuildRequires defined with a %bcond_with would be affected by this
issue. The current specfile only have %bcond_without conditional BuildRequires
which is fine.

Fixes: #15042
Signed-off-by: Erwan Velu erwan@redhat.com

By pull request 7742, the btrfs-progs package was considered as a BuildRequires
only when --with tests was engaged like :

    if %{with tests}
    BuildRequires: btrfsprogs
    %endif

That's perfectly valid for a spec file.

The issue we have is the following :
- yum-builddep called by install-deps.sh is used to prepare the build env by
installing the needed BuildRequires.
- %{with test} is defined by using a %bcond_with
- yum-builddep doesn't consider %{with test} as valid
- yum-builddep doesn't install the btrfs package

As per discussions with the yum team, there is no way to engage conditional flags with
yum-builddep.

So this patch, as per discussions with Nathan Cutler & Loic Dachary, is removing
the condition arond the BuildRequires.

Note that all BuildRequires defined with a %bcond_with would be affected by this
issue. The current specfile only have %bcond_without conditional BuildRequires
which is fine.

Fixes: ceph#15042
Signed-off-by: Erwan Velu <erwan@redhat.com>
@smithfarm
Copy link
Contributor

@ktdreyer What do you think about this approach? Another approach just occurred to me: the failing tests themselves could be re-done to not run if the relevant btrfs programs are not installed.

@ErwanAliasr1
Copy link
Contributor Author

@smithfarm but you cannot really get what is the underlying filesystem before running the test. At least, the current approach insures that it will work everywhere. btrfs-progs is not a big dep.

@smithfarm
Copy link
Contributor

@ErwanAliasr1 But I wrote "to not run if the relevant btrfs programs are not installed" not "to run only if the underlying filesystem is btrfs".

@ErwanAliasr1
Copy link
Contributor Author

@smithfarm I meant that if your underlying fs is btrfs and if the btrfs progs are not installed then many tests would be bypasses which is not very nice.

@smithfarm
Copy link
Contributor

OK, thanks for the clarification. Let's wait and see what the others think.

@ErwanAliasr1
Copy link
Contributor Author

Others ? What's your thoughts on that ?

@ErwanAliasr1
Copy link
Contributor Author

@ktdreyer any ideas on that issue we are discussing ?

@wjwithagen
Copy link
Contributor

On 16-3-2016 10:53, Erwan Velu wrote:

@ktdreyer https://github.com/ktdreyer any ideas on that issue we are
discussing ?

It would also require removing all the btrfs related code from the
testscripts. And digging into other sources to see where btrfs-isms are
being used.

If removing the dependancie on removing just a package is the purpose,
I'd say: why bother, if you're leaving the rest in.

my 2 cts,
--WjW

@ErwanAliasr1
Copy link
Contributor Author

@wjwithagen I've been fixing the btrfs issues in the test script and use them on a daily basis. So it works. Its just a matter of accepting keeping that build dep without any conditional statement.

@liewegas
Copy link
Member

dropping the conditional seems fine to me!

@ErwanAliasr1
Copy link
Contributor Author

So do I @liewegas. So the branch is ready.

@ErwanAliasr1
Copy link
Contributor Author

Can we merge that PR ?

@smithfarm
Copy link
Contributor

Reviewed-by: Nathan Cutler <ncutler@suse.com>

@liewegas liewegas changed the title Packaging: Removing conditional BuildRequires for btrfs-progs rpm,deb: Removing conditional BuildRequires for btrfs-progs Mar 21, 2016
@liewegas liewegas changed the title rpm,deb: Removing conditional BuildRequires for btrfs-progs rpm,deb: remove conditional BuildRequires for btrfs-progs Mar 21, 2016
liewegas added a commit that referenced this pull request Mar 21, 2016
rpm,deb: remove conditional BuildRequires for btrfs-progs

Reviewed-by: Nathan Cutler <ncutler@suse.com>
@liewegas liewegas merged commit 19aabf4 into ceph:master Mar 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants