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.

Sign up for GitHub

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

Jump to bottom

seafile-server: fix broken build on GCC 14 #370753

Merged
merged 1 commit into from
Jan 15, 2025
Merged

seafile-server: fix broken build on GCC 14 #370753

merged 1 commit into from
Jan 15, 2025

Conversation

melvyn2
Copy link
Contributor

@melvyn2 melvyn2 commented Jan 3, 2025 •
edited
Loading

Backports a patch from upstream to fix seafile-server's build on GCC 14 (#370748).

Also disables some tests for pysaml2 which were broken by an update to pyopenssl (IdentityPython/pysaml2#975), as it is a dependency of seafile.

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@nix-owners nix-owners bot requested review from greizgh and schmittlauch January 3, 2025 23:59
@melvyn2 melvyn2 force-pushed the seaf-server-hotpatch branch 2 times, most recently from 2ea02af to 4f69310 Compare January 4, 2025 04:40
@melvyn2 melvyn2 requested a review from getchoo January 4, 2025 04:42
@lucasew
Copy link
Contributor

lucasew commented Jan 4, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370753

x86_64-linux

❌ 11 packages failed to build:
  • matrix-synapse
  • matrix-synapse-plugins.matrix-synapse-ldap3
  • matrix-synapse-plugins.matrix-synapse-ldap3.dist
  • matrix-synapse-plugins.matrix-synapse-mjolnir-antispam
  • matrix-synapse-plugins.matrix-synapse-mjolnir-antispam.dist
  • matrix-synapse-plugins.matrix-synapse-s3-storage-provider
  • matrix-synapse-plugins.matrix-synapse-s3-storage-provider.dist
  • matrix-synapse-plugins.matrix-synapse-shared-secret-auth
  • matrix-synapse-plugins.matrix-synapse-shared-secret-auth.dist
  • matrix-synapse-unwrapped
  • matrix-synapse-unwrapped.dist
✅ 11 packages built:
  • python312Packages.djangosaml2
  • python312Packages.djangosaml2.dist
  • python312Packages.pysaml2
  • python312Packages.pysaml2.dist
  • seafile-server (python312Packages.seaserv)
  • python313Packages.djangosaml2
  • python313Packages.djangosaml2.dist
  • python313Packages.pysaml2
  • python313Packages.pysaml2.dist
  • python313Packages.seaserv
  • seahub

@melvyn2 melvyn2 force-pushed the seaf-server-hotpatch branch from 4f69310 to c33b887 Compare January 4, 2025 22:20
@melvyn2
Copy link
Contributor Author

melvyn2 commented Jan 4, 2025

Used the commit patch instead as requested. The build failure in matrix-synapse is seemingly unrelated to the pysaml2 fix, as I only disabled a few tests, and the failure is in some database tests.

@lucasew
Copy link
Contributor

lucasew commented Jan 5, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370753

x86_64-linux

❌ 11 packages failed to build:
  • matrix-synapse
  • matrix-synapse-plugins.matrix-synapse-ldap3
  • matrix-synapse-plugins.matrix-synapse-ldap3.dist
  • matrix-synapse-plugins.matrix-synapse-mjolnir-antispam
  • matrix-synapse-plugins.matrix-synapse-mjolnir-antispam.dist
  • matrix-synapse-plugins.matrix-synapse-s3-storage-provider
  • matrix-synapse-plugins.matrix-synapse-s3-storage-provider.dist
  • matrix-synapse-plugins.matrix-synapse-shared-secret-auth
  • matrix-synapse-plugins.matrix-synapse-shared-secret-auth.dist
  • matrix-synapse-unwrapped
  • matrix-synapse-unwrapped.dist
✅ 11 packages built:
  • python312Packages.djangosaml2
  • python312Packages.djangosaml2.dist
  • python312Packages.pysaml2
  • python312Packages.pysaml2.dist
  • seafile-server (python312Packages.seaserv)
  • python313Packages.djangosaml2
  • python313Packages.djangosaml2.dist
  • python313Packages.pysaml2
  • python313Packages.pysaml2.dist
  • python313Packages.seaserv
  • seahub

getchoo
getchoo approved these changes Jan 5, 2025
View reviewed changes
Copy link
Member

@getchoo getchoo left a comment

Choose a reason for hiding this comment

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

Diff LGTM. Thanks!

@getchoo getchoo added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 5, 2025
SuperSandro2000
SuperSandro2000 reviewed Jan 7, 2025
View reviewed changes
pkgs/development/python-modules/pysaml2/default.nix Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Can you please rebase on master? I think we need to do some changes to pysaml2 as it got marked broken in #369859

SuperSandro2000
SuperSandro2000 previously requested changes Jan 7, 2025
View reviewed changes
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.

must be rebased on master

@melvyn2 melvyn2 force-pushed the seaf-server-hotpatch branch from c33b887 to 703d042 Compare January 7, 2025 15:46
@JohnRTitor
Copy link
Contributor

CC #371815

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 7, 2025
@lucasew
Copy link
Contributor

lucasew commented Jan 7, 2025

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370753

x86_64-linux

⏩ 9 packages marked as broken and skipped:
  • python312Packages.djangosaml2
  • python312Packages.djangosaml2.dist
  • python312Packages.pysaml2
  • python312Packages.pysaml2.dist
  • python313Packages.djangosaml2
  • python313Packages.djangosaml2.dist
  • python313Packages.pysaml2
  • python313Packages.pysaml2.dist
  • seahub
✅ 2 packages built:
  • seafile-server (python312Packages.seaserv)
  • python313Packages.seaserv

jcaesar
jcaesar reviewed Jan 8, 2025
View reviewed changes
pkgs/development/python-modules/pysaml2/default.nix Outdated
# https://github.com/IdentityPython/pysaml2/issues/975
"test_encrypted_response_6"
"test_validate_cert_chains"
"test_validate_with_root_cert"
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better not to skip the pysaml2 tests, that package is actually broken independently of seafile-server. See #367976 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's marked as broken already, so it should be fine to do the minimum to make it build, no? I don't think it can be removed as a dependency for seafile-server, even if it's not actually used.

Copy link
Contributor

@jcaesar jcaesar Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm sorry, I noticed I'm lost. If my comment doesn't make sense feel free to ignore it, but it appears seafile-server doesn't depend on pysaml2 at all? nix build github:NixOS/nixpkgs/3d8dc15ab30b5fa6f20e42c357817e337ee0ba3a#seafile-server (i.e. skipping the second commit of your PR) seems to build just fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, that's my mistake -- it's seahub that depends on it. I'd cherry-picked this from the update PR because it's necessary to pass the nixos test, but the commit isn't strictly required for fixing just the build.

I guess it doesn't really belong in this commit.

Copy link
Contributor

@jcaesar jcaesar Jan 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Ah. I see. Yeah indeed, you probably want a separate PR for seahub / pysaml2.

This PR is all good now.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 370753

x86_64-linux

⏩ 1 package marked as broken and skipped:
  • seahub
✅ 2 packages built:
  • seafile-server (python312Packages.seaserv)
  • python313Packages.seaserv

@melvyn2 melvyn2 force-pushed the seaf-server-hotpatch branch from 703d042 to 3d8dc15 Compare January 8, 2025 04:25
@github-actions github-actions bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 6.topic: python 10.rebuild-darwin: 1-10 labels Jan 8, 2025
@JohnRTitor JohnRTitor merged commit fcc6b7d into NixOS:master Jan 15, 2025
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnRTitor JohnRTitor JohnRTitor left review comments

@getchoo getchoo getchoo approved these changes

@jcaesar jcaesar jcaesar approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@greizgh greizgh Awaiting requested review from greizgh

@schmittlauch schmittlauch Awaiting requested review from schmittlauch

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@melvyn2 @lucasew @JohnRTitor @jcaesar @SuperSandro2000 @getchoo @wegank