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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

onlyoffice-bin_latest: 7.5.1 -> 8.0.0 #285888

Merged
merged 2 commits into from
Feb 11, 2024
Merged

Conversation

fin444
Copy link
Contributor

@fin444 fin444 commented Feb 2, 2024

Description of changes

Changelog

ONLYOFFICE is still broken on wlroots. Thus, only the _latest package is being updated to preserve compatibility.

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.05 Release Notes (or backporting 23.05 and 23.11 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.

@fin444
Copy link
Contributor Author

fin444 commented Feb 2, 2024

The way the split-package setup was handled meant that it needed to be renamed every update, OfBorg interpreted this as a new package and it tripped the new test for pkgs/by-name. It seemed to me the best way to handle this was just to move ONLYOFFICE over to the new scheme. I think I did it correctly?

@infinisil
Copy link
Member

I believe this was a false positive in the pkgs/by-name check (notably that's not ofborg). There's already a PR that would fix it: #285089

So there's no need to migrate this to pkgs/by-name. And indeed I wouldn't recommend it especially because of the complexity with multiple package versions, which pkgs/by-name can't deal with yet.

@fin444
Copy link
Contributor Author

fin444 commented Feb 2, 2024

If I am understanding your PR correctly, it is designed to fix cases where the two versions are defined by the same file, called with different arguments? That is not the case here, the two versions have very different setups and so use different files.

@infinisil
Copy link
Member

Ohh sorry you're right, this is not a case where that PR would've helped. Looking at the first commit in this PR (42a8cab), for all CI can see, onlyoffice-bin_8_0 is indeed a new package using callPackage, therefore it would have to go into pkgs/by-name.

On one hand, if you have completely separate expressions, migration to pkgs/by-name sounds good. However in this case I do see the potential to refactor these to have more shared code, which then wouldn't work the same in pkgs/by-name.

I'd say that's up to the maintainers, especially also the decision of whether it makes sense to keep pinned old versions around.

Something generally important though: Don't remove top-level attributes without adding at least a throw alias in aliases.nix to say why it got removed.

@fin444
Copy link
Contributor Author

fin444 commented Feb 3, 2024

ONLYOFFICE versions past 7.2 are completely inoperable on wlroots (and they don't seem interested in fixing it), which is why the package was split in the first place. onlyoffice-bin is kept as-is for compatibility, while onlyoffice-bin_latest is updated. Over time, I would expect the two to continue to diverge, so I'm not sure combining them would be good in the long term. (And the idea is that if they were to decided to fix it we could just make the newer version be the definitive one and un-split them)

But you are correct on the throw alias, I have now added it.

@infinisil
Copy link
Member

Over time, I would expect the two to continue to diverge, so I'm not sure combining them would be good in the long term

Sounds good to keep them separate then!

@fin444
Copy link
Contributor Author

fin444 commented Feb 5, 2024

Also on the subject of update.sh - I copied it into both since that's how it was before. I'm not entirely sure how these work, but I don't really know if this was the correct move? They also just don't seem to have been working in the first place, even before the package split.

@GTrunSec
Copy link
Contributor

GTrunSec commented Feb 5, 2024

I'm not entirely sure how these work, but I don't really know if this was the correct move?

@fin444, you can try the command as follows to check whether the update script works.

nix-shell maintainers/scripts/update.nix --argstr package onlyoffice-<version>

@fin444
Copy link
Contributor Author

fin444 commented Feb 5, 2024

Ah, I was under the impression they were there for r-ryantm, not for the maintainers. They do still work, but looks like I broke _latest's when updating to 7.4 because I put the passthru in the wrong spot. Have fixed that now.

I'm learning in this PR 馃槅

Copy link
Contributor

@GTrunSec GTrunSec left a comment

Choose a reason for hiding this comment

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

LGTM

@wegank wegank merged commit d8eab17 into NixOS:master Feb 11, 2024
24 checks passed
@fin444 fin444 deleted the onlyoffice-8.0.0 branch February 24, 2024 22:53
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.

None yet

5 participants