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

tracker: patch failing test due to float comparison #141796

Merged
merged 1 commit into from
Oct 16, 2021
Merged

tracker: patch failing test due to float comparison #141796

merged 1 commit into from
Oct 16, 2021

Conversation

fabianhjr
Copy link
Member

@fabianhjr fabianhjr commented Oct 15, 2021 •
edited
Loading

Motivation for this change

Fixes #141728, #140828 (comment), #139144 (comment), #139144 (comment)

Tested building tracker locally successfully.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@fabianhjr fabianhjr mentioned this pull request Oct 15, 2021
Merged
@fabianhjr fabianhjr mentioned this pull request Oct 15, 2021
Merged
12 tasks
jtojnar
jtojnar reviewed Oct 15, 2021
View reviewed changes
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Could you also please try to upstream the patch?

pkgs/development/libraries/tracker/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/tracker/test.patch Outdated Show resolved Hide resolved
@jtojnar jtojnar changed the title gnome.tracker: patch failing test due to float comparison tracker: patch failing test due to float comparison Oct 15, 2021
@fabianhjr
Copy link
Member Author

Could you also please try to upstream the patch?

Yes, do you want to be added as a co-authored-by on the upstream patch?

@jtojnar
Copy link
Member

jtojnar commented Oct 15, 2021

Could you also please try to upstream the patch?

Yes, do you want to be added as a co-authored-by on the upstream patch?

No need for something so trivial, I doubt it would even be copyrightable.

@fabianhjr fabianhjr force-pushed the patch-tracker branch 4 times, most recently from 622fe0a to f9c9ec1 Compare October 15, 2021 21:20
@fabianhjr
Copy link
Member Author

Upstreamed: https://gitlab.gnome.org/GNOME/tracker/-/merge_requests/469

Using fetchpatch instead of commiting a patch directly

@ofborg ofborg bot requested review from amaxine, dasj19, hedning and jtojnar October 16, 2021 03:41
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100 labels Oct 16, 2021
chvp
chvp approved these changes Oct 16, 2021
View reviewed changes
Copy link
Member

@chvp chvp left a comment

Choose a reason for hiding this comment

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

This allowed me to update my system and the application that (transitively) depended on it (steam) runs without problems.

@fabianhjr fabianhjr changed the title tracker: patch failing test due to float comparison [staging-next] tracker: patch failing test due to float comparison Oct 16, 2021
@davidak
Copy link
Member

davidak commented Oct 16, 2021

Result of nixpkgs-review pr 141796 run on x86_64-linux 1

1 package built:
  • steam

@davidak
Copy link
Member

davidak commented Oct 16, 2021

It was merged upstream, so should be good enough for us to merge.

I'm a bit skeptical because i had the issue also on x86_64, but i will test again when it's merged.

@jtojnar
Copy link
Member

jtojnar commented Oct 16, 2021 •
edited
Loading

Could you please also rebase and switch the target branch to master?

@fabianhjr fabianhjr changed the base branch from staging-next to master October 16, 2021 14:40
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 16, 2021
@github-actions github-actions bot removed 8.has: changelog 6.topic: printing 6.topic: kernel The Linux kernel 6.topic: systemd 8.has: documentation This PR adds or changes documentation 6.topic: qt/kde 6.topic: python 6.topic: stdenv Standard environment 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: rust 8.has: module (update) This PR changes an existing module in `nixos/` labels Oct 16, 2021
@ofborg ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 16, 2021
@fabianhjr
Copy link
Member Author

Could you please also rebase and switch the target branch to master?

Switch to target master

@jtojnar jtojnar merged commit d4aa99a into NixOS:master Oct 16, 2021
@jtojnar
Copy link
Member

jtojnar commented Oct 16, 2021

Thanks.

@fabianhjr fabianhjr deleted the patch-tracker branch October 16, 2021 15:37
SuperSandro2000
SuperSandro2000 reviewed Oct 16, 2021
View reviewed changes
pkgs/development/libraries/tracker/default.nix
@@ -44,6 +45,13 @@ stdenv.mkDerivation rec {
src = ./fix-paths.patch;
inherit asciidoc;
})
] ++ lib.optionals (stdenv.hostPlatform.isi686) [
Copy link
Member

Choose a reason for hiding this comment

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

First of all the () are optional and should be removed. Second no one ever tests packages on i686 when updating which means the patch will bit rot almost immediately. Why is this not applied unconditional?

Copy link
Member

@jtojnar jtojnar Oct 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Steam depends on i686 and applying it unconditionally would cause a mass rebuild.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it will bit-rot, it won't apply on the next upstream tracker release and many nix contributors/maintainers depend on i686 build of tracker trough transitive dependencies like steam.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though, also @jtojnar is one of the two persons on the GNOME maintainers team and has been putting a lot of work into updates. Regardless I will be on the lookout for the next version bump.

Copy link
Member

Choose a reason for hiding this comment

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

If steam depends on it, it should be fine.

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

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@chvp chvp chvp approved these changes

@TredwellGit TredwellGit Awaiting requested review from TredwellGit

@maxhille maxhille Awaiting requested review from maxhille

@davidak davidak Awaiting requested review from davidak

@amaxine amaxine Awaiting requested review from amaxine

@dasj19 dasj19 Awaiting requested review from dasj19

@hedning hedning Awaiting requested review from hedning

@jtojnar jtojnar Awaiting requested review from jtojnar

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

Successfully merging this pull request may close these issues.

tracker fails to build on some systems (test failure)
5 participants
@fabianhjr @jtojnar @davidak @SuperSandro2000 @chvp