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

Fix Fedora and CentOS builds #3246

Merged
merged 2 commits into from
Dec 29, 2021
Merged

Conversation

thornbill
Copy link
Member

Changes

  • Fixes Fedora builds failing due to incorrect version number
  • Fixes CentOS builds failing due to permission issues

Issues
Fixes (part of) #3222

@thornbill thornbill added regression We broke something build This PR or issue mainly concerns build tools labels Dec 29, 2021
@sonarcloud
Copy link

sonarcloud bot commented Dec 29, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@crobibero crobibero merged commit 4e3e91f into jellyfin:master Dec 29, 2021
@thornbill thornbill deleted the fix-ci-builds branch December 29, 2021 20:51
@@ -30,6 +30,10 @@ Jellyfin is a free software media system that puts you in control of managing an
%build

%install
%if 0%{?rhel} > 0 && 0%{?rhel} < 8
# Required for CentOS build
chown root:root -R .
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still going to break any properly built RPM build system.

You cannot do root things in an RPM scriptlet and SHOULD NOT build RPMs as root.

I am working on fixing all of these issues in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Brian, I'm going to be as blunt as I can, because I think that's the only way to get this point across clearly at this point. We do not care what you say we can and cannot do, or what's "proper". We care what works. And given that this has been broken for well over a month due to your previous "improvements", we're going to go with what works. Despite pointing it out to you and asking repeatedly for you to fix it, you've argued about it and denied that it was a problem. This whole situation has (again) prompted discussions about removing official support for rpm systems since none of us use them and it's brought the whole build system to it's knees.

TLDR: Until you're willing to put in the work and hand us something that is guaranteed to work properly and not over complicate things more than they already are, stop criticizing. Your previous efforts are leaving something to be desired

@brianjmurrell
Copy link
Contributor

brianjmurrell commented Dec 30, 2021

We do not care what you say we can and cannot do, or what's "proper".

These are NOT my opinions. Maybe these links are from organizations who don't know what they are talking about either?

https://wiki.centos.org/HowTos/SetupRpmBuildEnvironment#:~:text=Building%20RPMs%20should%20NEVER%20be,root%20might%20damage%20your%20system.

https://docs.fedoraproject.org/en-US/Fedora_Draft_Documentation/0.1/html/RPM_Guide/ch13s02s02s02.html

We care what works.

But it doesn't. As everyone who will try to rebuild your SRPM will find out, it will be broken, because they won't try to rebuild it as root. So why bother trying to build something that will not work for anyone who wants to try to rebuild it?

broken for well over a month due to your previous "improvements",

Except the PR that made the changes did not show anything broken and nobody said anything was broken until a week ago. So why didn't the PR with the "improvement" break? It would have been immensely helpful if it had because then the root issues would have been more apparent and the "improvement" more thorough to fix the other issues as well, before it was landed.

Until you're willing to put in the work and hand us something that is guaranteed to work properly

Did you miss the part of my previous message: I am working on fixing all of these issues in another PR? So I am not just standing on sidelines criticizing. I am trying to fix the problems. And I do have them fixed, and working in Docker here, but the Docker that is on Azure seems to be a little broken -- or at least different in how it handles mapped directories. I only discovered this today and am still investigating it.
.

@joshuaboniface
Copy link
Member

joshuaboniface commented Dec 31, 2021

I appreciate that you are trying to fix this Brian, I truly do. But please look at it from our perspective:

  1. These systems have been built ad-hoc for over 3 years now to try to get binary packages to our users. If someone is building from the SRPMS on their own, that's cool, but we don't officially support doing that without using the Docker wrapper, which is precisely what the CI is running too.
  2. We've crafted this to work with our CI. Is our CI good? No, it's jank, and we've been working on attempt 3 to replace it.
  3. But we need to get 10.8.0 out. It's been nearly a year since 10.7.0. We need a new release cut from our master. We've been trying to get that going for several months now, after which we can resume focus on improving our CI.
  4. Thus, I'm really not interested in completely breaking what "works" (for us, with our official CI) right now at the 11th hour. Hell even the bump_version stuff was only for our prereleases - nothing else, not stable releases or point releases after - all of that works and we know it does since it wasn't changed since 10.7.0. But we need to get post-10.8.0 to focus on real fixes/breakage or we're just going to delay this even longer. That's what even prompted all this, a small change to make my triggering of alpha4/beta1/rc1/etc. easier on me (and not a 4+ hour ordeal on our build server), but the surrounding "correctness" changes have broken what we have right now for unstable builds (still a very important part of testing right now) and thus, we need to revert. The "real" stuff can come after.

I'll reiterate again - I do appreciate your fixes, and I want to get "native ready" packages along with the new CI after 10.8.0, including a full separation of our "OS packages" from the main code repo like most packages out there. And I'll value your input greatly on that (heck we can make COPR the real default build for our Fedora/CentOS users), but after 10.8.0.

@brianjmurrell
Copy link
Contributor

Hi @joshuaboniface. #3238 fixes all of the issues. It also adds package building to PRs in CI, since if that were the case when my original PR went through CI, nothing would have broken and the issues with the package build in CI would have been much more clear.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build This PR or issue mainly concerns build tools regression We broke something
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants