Skip to content

browsr: 1.19.0 -> 1.21.0, mupdf: 1.23.6 -> 1.24.8, python312Packages.pymupdf: 1.23.6 -> 1.24.8, python312Packages.textual-universal-directory: 1.1.0 -> 1.5.0#332385

Merged
thiagokokada merged 5 commits intoNixOS:stagingfrom
greg-hellings:browsr-1.21.0
Aug 14, 2024

Conversation

@greg-hellings
Copy link
Contributor

@greg-hellings greg-hellings commented Aug 5, 2024

Description of changes

A change in a previous PR lifted a Python 3.12 limitation on building browsr. The existing version then failed to build due to being out of date. This PR updates the rest of the dependency tree to build in Python 3.12 and to reflect their latest versions.

Closes #274882
Closes #290651

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: python Python is a high-level, general-purpose programming language. label Aug 5, 2024
@ofborg ofborg bot requested review from figsoda, fpletz and teto August 5, 2024 04:00
@ofborg ofborg bot added 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Aug 5, 2024
@fabaff fabaff linked an issue Aug 5, 2024 that may be closed by this pull request
@teto
Copy link
Member

teto commented Aug 5, 2024

Looks fine. Considering the number of rebuild, this should probably target staging though

@phanirithvij
Copy link
Member

what about #298783 #274882 #290651 etc. can they all be closed?

@greg-hellings
Copy link
Contributor Author

what about #298783 #274882 #290651 etc. can they all be closed?

It looks like they would be able to be closed if this gets merged

@phanirithvij
Copy link
Member

Could you update the target branch for the pr to staging from master?

@greg-hellings greg-hellings changed the base branch from master to staging August 6, 2024 20:20
@greg-hellings
Copy link
Contributor Author

Could you update the target branch for the pr to staging from master?

Done

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Can you also pick f9eb925 (#298783)

Also there are some additional changes in b476b96 (#298783) and 4a3db5a (#298783) which I think are worthwhile to carry over to here

@SuperSandro2000 SuperSandro2000 mentioned this pull request Aug 14, 2024
13 tasks
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@thiagokokada thiagokokada merged commit e0626c7 into NixOS:staging Aug 14, 2024
@thiagokokada
Copy link
Contributor

Can you also pick f9eb925 (#298783)

Also there are some additional changes in b476b96 (#298783) and 4a3db5a (#298783) which I think are worthwhile to carry over to here

I went and merged this since while those changes are nice to have, they're not essential, and mupdf is broken for a long time in nixpkgs so it is better to fix it and improve the package later.

@dotlambda
Copy link
Member

I went and merged this since while those changes are nice to have, they're not essential, and mupdf is broken for a long time in nixpkgs so it is better to fix it and improve the package later.

Then why were the other PRs closed??

@thiagokokada
Copy link
Contributor

I went and merged this since while those changes are nice to have, they're not essential, and mupdf is broken for a long time in nixpkgs so it is better to fix it and improve the package later.

Then why were the other PRs closed??

Because the main thing that is the bump is done, the only changes are smaller ones that can be done targeting either master or staging.

I think at that point it will be better to just remake the PR since there are lots of conflicts.

@dotlambda
Copy link
Member

I think at that point it will be better to just remake the PR since there are lots of conflicts.

They can be closed once that's done.
This is unacceptable.

@thiagokokada
Copy link
Contributor

I think at that point it will be better to just remake the PR since there are lots of conflicts.

They can be closed once that's done. This is unacceptable.

Feel free to reopen the PRs if you think I did a mistake. I only closed them to help reduce the huge amount of PRs we have.

No need to feel attacked.

@SuperSandro2000
Copy link
Member

and mupdf is broken for a long time in nixpkgs so it is better to fix it and improve the package later.

If it is broken for a longer time and there is no urgency, we can also take the extra day or two to take the improvements.

Also apparently this PR was so rushed, that we immediately got a regression in #334596

@dotlambda
Copy link
Member

dotlambda commented Aug 15, 2024

Also apparently this PR was so rushed, that we immediately got a regression in #334596

That's unrelated because this PR wasn't merged into master.

@thiagokokada
Copy link
Contributor

Also apparently this PR was so rushed, that we immediately got a regression in #334596

Completely unrelated, this PR is targetting staging.

@thiagokokada
Copy link
Contributor

If it is broken for a longer time and there is no urgency, we can also take the extra day or two to take the improvements.

It is my opinion that sometimes we focus too much in fixing things that doesn't really help anyone. For example, the meta.mainProgram change, it is fine, but it is something so trivial that can be done in a separate PR targetting master directly. No need to go through staging, and no need to be backported to this PR.

IMO, this kind ends up just discouraging newcomers that end up doing good work fixing difficult packages like this one. But I am not sure if this is the correct place to discuss this issue either, feel free to ask me in Matrix or something if you want to discuss this issue further.

@SuperSandro2000
Copy link
Member

If the change is so trivial IMO creating another PR is just creating overhead for reviewers and mergers.

@thiagokokada
Copy link
Contributor

If the change is so trivial IMO creating another PR is just creating overhead for reviewers and mergers.

I think the overhead is way higher for the ones opening a PR than the ones reviewing it. And at least for low impact changes, I don't think it matters too much. Now, getting packages fixed has a much higher impact, IMO.

But we can agree to disagree here, not sure if it makes sense to comment much more on this particular issue. If I don't forget I will open a PR later applying the meta.mainProgram changes.

@greg-hellings greg-hellings deleted the browsr-1.21.0 branch August 15, 2024 14:49
@greg-hellings
Copy link
Contributor Author

If the change is so trivial IMO creating another PR is just creating overhead for reviewers and mergers.

It also makes it an absolute PITB for someone who just wants to contribute a fix and isn't wanting to be nitpicked to death by drift in reviewer's preferences and optional fields since the package was first merged in. In this case, adding a mainProgram option would be trivial, but would require bouncing back through a git rebase that not every contributor would be comfortable with. The other two PRs linked make major changes to the mupdf package. If those changes are desired, they should be subject to a completely different process than just a quick build fix submission by someone who is not the package maintainer and who just needs the package to build in order to fix their own problem. I just wanted browsr to work, I didn't want to adopt mupdf. I just needed to fix mupdf in order to get there.

@vcunat
Copy link
Member

vcunat commented Aug 30, 2024

FYI: 9b88d05

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 1001-2500 This PR causes many rebuilds on Darwin and should most likely target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

python3*Packages.pymupdf: Runtime import failure in buildEnv

7 participants