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: build-depends on "cunit-devel" for suse #18997

Merged
merged 1 commit into from Nov 24, 2017

Conversation

Projects
None yet
4 participants
@tchaikov
Copy link
Contributor

commented Nov 18, 2017

No description provided.

rpm: add cunit-devel for 'suse_version'
Signed-off-by: Kefu Chai <kchai@redhat.com>

@tchaikov tchaikov requested review from smithfarm and b-ranto Nov 18, 2017

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2017

I don't know. . . I like having the distro-specific code blocks as self-contained wholes (IIRC we had if/else before and it was overly complicated and hard to figure out, especially when nesting of if/else blocks was involved). Unless there's a specific bug or problem with the current code, I don't see a compelling reason to refactor it. @b-ranto, @ktdreyer Your thoughts?

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

commented Nov 18, 2017

@smithfarm could you help me understand how the self-contained code helps to avoid the overly complicated rpm recipe block? a commit/PR/comment/article/code excerpt would suffice.

i think it'd be better to be explicit about mutual exclusive conditions. yes, this refactor commit is not for fixing a bug or a problem, it's just a cosmetic change. if none of you is interested in it, and believes it's bad. i am going to drop it.

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Nov 18, 2017

@tchaikov I don't think the idea is bad, per se. In a real programming language (with subroutines and indentation) your way would be cleaner. But given the limitations of RPM syntax, I've found it's better to keep the if blocks as small as possible. Still, I'm interested to hear what others think.

@tchaikov tchaikov requested a review from ktdreyer Nov 22, 2017

@ktdreyer
Copy link
Member

left a comment

on the %elseif thing, IMHO, fewer lines in the .spec make it easier to read

@smithfarm

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2017

Any use of %else (including %elseif) can potentially give rise to confusion when %if blocks are nested (and we have had buggy spec file patches for this reason in the past), but if this patch makes the code easier to read for someone, I drop my objection.

@tchaikov tchaikov added the needs-qa label Nov 23, 2017

@smithfarm
Copy link
Contributor

left a comment

Unfortunately, %ifelse breaks the OBS build. Before triggering rpm itself, OBS scans the spec file to determine build dependencies. Obviously, the OBS code that does this does not support %ifelse :-(

Granted, this is a bug in OBS. I will try to get it fixed ASAP, but until I can do that can we please drop the %ifelse commit.

@tchaikov tchaikov force-pushed the tchaikov:wip-rpm-cunit-for-suse branch from bb99f71 to 1ac89cb Nov 24, 2017

@tchaikov

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2017

@smithfarm okay, i am creating #19126, and assigning it to you. so once the OBS bug gets fixed, we can test and merge it.

@smithfarm smithfarm merged commit 267a145 into ceph:master Nov 24, 2017

4 of 5 checks passed

make check (arm64) make check failed
Details
Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details

@smithfarm smithfarm referenced this pull request Nov 24, 2017

Closed

SES6 initial build #168

@tchaikov tchaikov deleted the tchaikov:wip-rpm-cunit-for-suse branch Nov 24, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.