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

Add initial support for building sambacc rpms via COPR #78

Merged
merged 9 commits into from
Apr 21, 2023

Conversation

phlogistonjohn
Copy link
Collaborator

Depends on: #77

Another grab bag of changes that work up to getting basic support for COPR in place.
Prepare sambacc and the rpm spec file for optional dependencies.
Fix the rpm spec generation to work correctly outside our container env.
Make the build.sh script flexible enough to be reused in the COPR environment. Add a .copr/Makefile for the COPR system to build SRPMs directly from the git checkout.

@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 18, 2023

@Mergifyio rebase

@dpulls
Copy link

dpulls bot commented Apr 18, 2023

🎉 All dependencies have been resolved !

@mergify
Copy link

mergify bot commented Apr 18, 2023

rebase

❌ Unable to rebase: user obnoxxx is unknown.

Please make sure obnoxxx has logged in Mergify dashboard.

@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 18, 2023

@Mergifyio rebase

@mergify
Copy link

mergify bot commented Apr 18, 2023

rebase

✅ Branch has been successfully rebased

@phlogistonjohn phlogistonjohn marked this pull request as ready for review April 18, 2023 18:26
@phlogistonjohn phlogistonjohn changed the title [WIP] Add initial support for building sambacc rpms via COPR Add initial support for building sambacc rpms via COPR Apr 18, 2023
@phlogistonjohn
Copy link
Collaborator Author

I've added the do-not-merge label to block automerging until samba-container is able to correctly deal with >1 rpm in the "distdir". Please do review the code!

@obnoxxx
Copy link
Contributor

obnoxxx commented Apr 18, 2023

@phlogistonjohn wrote:

I've added the do-not-merge label to block automerging until samba-container is able to correctly deal with >1 rpm in the "distdir". Please do review the code!

I do not quite get the connection to samba-container .... 😬

@phlogistonjohn
Copy link
Collaborator Author

@phlogistonjohn wrote:

I've added the do-not-merge label to block automerging until samba-container is able to correctly deal with >1 rpm in the "distdir". Please do review the code!

I do not quite get the connection to samba-container .... grimacing

Right now, sambacc gets installed in samba-container images by buidling rpms in a build container and then they get installed by a script that checks that there's only one RPM in the results dir. This PR adds a subpackage to sambacc RPM and so every build will produce at least 2 rpms. We need to make sure that this change wont just break the install script. I was just looking at the script and I think it might be OK beceause the secondary rpms that get created have plus signs (+) in the name where the script only assumes a dash should be. But I haven't tested it yet.

@phlogistonjohn phlogistonjohn removed the do-not-merge Hold on label Apr 18, 2023
@phlogistonjohn
Copy link
Collaborator Author

Locally building samba-container server using this branch for sambacc worked OK for me. Removing label.

Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

I don't know enough python to provide a deeper review but some typos (I guess) caught my attention . Apart from those, the patches look good to me.Apart from those, the changes look good to me

sambacc/config.py Outdated Show resolved Hide resolved
sambacc/config.py Outdated Show resolved Hide resolved
If jsonschema module doesn't understand our schemas then we assume that
the version is too old and we should treat it the same as not having
jsonschema installed. We recently found that having older jsonschema
versions present in the container actually breaks sambacc. Because we
prefer to install from rpm packages rather than pip in samba-container
we ended up with older, and an untested version, of the library. This
change tries to make the sambacc code more robust in that case.

Signed-off-by: John Mulligan <[email protected]>
extras/python-sambacc.spec Outdated Show resolved Hide resolved
extras/python-sambacc.spec Outdated Show resolved Hide resolved
.copr/Makefile Outdated Show resolved Hide resolved
.copr/Makefile Outdated Show resolved Hide resolved
.copr/Makefile Show resolved Hide resolved
Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

my previous request has been addressed. remaining request is to fix the typo that @anoopcs9 had already mentioned ...

extras/python-sambacc.spec Outdated Show resolved Hide resolved
When a generated srpm was submitted to COPR it failed due to the
check phase running the tests and failing due to missing samba
components. Explicitly add some of those components to the spec.

Signed-off-by: John Mulligan <[email protected]>
Clean up how the arguments passed to rpmbuild are written.

Signed-off-by: John Mulligan <[email protected]>
When using the container to build "binary" rpms passing the pversion and
rversion vars mostly worked, but the srpm it generated was not usable.
This change generates a new spec file based on the old, but with
pversion and rversion predefined, based on the same setuptools_scm
values, such that the srpm is usable in other environments.

Signed-off-by: John Mulligan <[email protected]>
This is considered the flexible way to build in COPR and it meets our
needs to match the setuptools_scm versionoing scheme we've already been
using for a while.  Hackily reuse the container build.sh script for
generating the srpm.

Signed-off-by: John Mulligan <[email protected]>
@phlogistonjohn
Copy link
Collaborator Author

@anoopcs9 @obnoxxx ready for a new round of reviews.

Overall, this PR is still a bit hacky and I'd appreciate it if you could excuse some of the hackyness I want to get a "minimal viable" PR that gets COPR up and working. I plan to clean up and refactor the build process later on FWIW

@anoopcs9
Copy link
Collaborator

I plan to clean up and refactor the build process later on FWIW

Much appreciated.

@anoopcs9 anoopcs9 requested review from obnoxxx and removed request for obnoxxx April 21, 2023 04:31
Copy link
Contributor

@obnoxxx obnoxxx left a comment

Choose a reason for hiding this comment

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

lgtm

@mergify mergify bot merged commit 34d81c9 into samba-in-kubernetes:master Apr 21, 2023
@phlogistonjohn phlogistonjohn deleted the jjm-copr-prep branch April 21, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants