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

ente-auth: fix build failure because of wrong icon path #352749

Merged
merged 1 commit into from
Nov 2, 2024

Conversation

MrSom3body
Copy link
Contributor

@MrSom3body MrSom3body commented Nov 1, 2024

Fix build failure for ente-auth.

ZHF: #352882

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.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 1, 2024
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution!

@MrSom3body can you share more details about what are you trying to fix? This package builds for me on master, but I get this error with your change:

install: cannot stat '/nix/store/dxgqlaxsjydgxshh29f0qsz7jszlkdgs-ente-auth-4.0.2/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

To test the build yourself, you can run nix-build -A ente-auth from the root of your fork of nixpkgs.

Comment on lines +81 to 84
FAV=$out/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png
ICO=$out/share/icons

install -D $FAV $ICO/ente-auth.png
Copy link
Member

@Aleksanaa Aleksanaa Nov 1, 2024

Choose a reason for hiding this comment

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

Suggested change
FAV=$out/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png
ICO=$out/share/icons
install -D $FAV $ICO/ente-auth.png
install -D $out/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png -t $out/share/pixmaps

Delete the below lines about imagemagick. These should be useless

Copy link
Contributor

Choose a reason for hiding this comment

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

ICO and FAV are also used in a for loop right below, I think its cleaner to keep those variables.

Copy link
Member

Choose a reason for hiding this comment

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

I'm saying the following should be removed. share/pixmaps is stipulated by the freedesktop specification, and this path does not need to specify the size. I don't think zooming with imagemagick is a good way.

Also remove imagemagick from nativeBuildInputs and function formal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for explaining this! Even though pixmaps is deprecated, it seems like better choice than creating images with imagemagick.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to a follow-up PR as it's irrelevant to this fix, so we can unbreak the build first.

@MrSom3body
Copy link
Contributor Author

MrSom3body commented Nov 1, 2024

Thanks for your contribution!

@MrSom3body can you share more details about what are you trying to fix? This package builds for me on master, but I get this error with your change:

install: cannot stat '/nix/store/dxgqlaxsjydgxshh29f0qsz7jszlkdgs-ente-auth-4.0.2/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

To test the build yourself, you can run nix-build -A ente-auth from the root of your fork of nixpkgs.

Huh, that's weird, I get the same error message if I do not have the change. Here's my log.

Going into the directory that it build, I can see the following directory structure:
2024-11-01T12:59:46,740490739+01:00

@MrSom3body MrSom3body changed the title ente-auth: fix icon path ente-auth: fix build failure because of wrong icon path Nov 1, 2024
@niklaskorz
Copy link
Contributor

Thanks for your contribution!
@MrSom3body can you share more details about what are you trying to fix? This package builds for me on master, but I get this error with your change:

install: cannot stat '/nix/store/dxgqlaxsjydgxshh29f0qsz7jszlkdgs-ente-auth-4.0.2/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

To test the build yourself, you can run nix-build -A ente-auth from the root of your fork of nixpkgs.

Huh, that's weird, I get the same error message if I do not have the change. Here's my log.

Going into the directory that it build, I can see the following directory structure:

Are you also using Lix, like the original reporter of the build failure?

@MrSom3body
Copy link
Contributor Author

Yes, I am. So it seems to be a problem with lix and not with the package itself. Should I close the pull request then?

@zi3m5f
Copy link
Contributor

zi3m5f commented Nov 1, 2024

install: cannot stat '/nix/store/q0ckhxy060rb82714z391xl0gfisqrsp-ente-auth-4.0.2/app/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

I get this error also with nix on both nixos-unstable-small and master. Tested with:

  • nix-2.18.8
  • nix-2.24.9
  • lix-2.90.0
  • lix-2.91.1

(I am also a lix user but tested and build in nix-shell -p nixVersions.nix_2_18 etc)

@niklaskorz
Copy link
Contributor

install: cannot stat '/nix/store/q0ckhxy060rb82714z391xl0gfisqrsp-ente-auth-4.0.2/app/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

I get this error also with nix on both nixos-unstable-small and master. Tested with:

* nix-2.18.8

* nix-2.24.9

* lix-2.90.0

* lix-2.91.1

(I am also a lix user but tested and build in nix-shell -p nixVersions.nix_2_18 etc)

Spawning a shell is insufficient. The Nix daemon is unaffected by this.

@zi3m5f
Copy link
Contributor

zi3m5f commented Nov 1, 2024

install: cannot stat '/nix/store/q0ckhxy060rb82714z391xl0gfisqrsp-ente-auth-4.0.2/app/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

I get this error also with nix on both nixos-unstable-small and master. Tested with:

* nix-2.18.8

* nix-2.24.9

* lix-2.90.0

* lix-2.91.1

(I am also a lix user but tested and build in nix-shell -p nixVersions.nix_2_18 etc)

Spawning a shell is insufficient. The Nix daemon is unaffected by this.

Sorry, didn't think.
I tested an fedora, my daemon is from nix-2.21.2 🤷‍♂️

@zi3m5f
Copy link
Contributor

zi3m5f commented Nov 1, 2024

#345404 changed the installPhase of buildFlutterApplication.
Reverting fixes the error :)

@niklaskorz
Copy link
Contributor

install: cannot stat '/nix/store/q0ckhxy060rb82714z391xl0gfisqrsp-ente-auth-4.0.2/app/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory

I get this error also with nix on both nixos-unstable-small and master. Tested with:

* nix-2.18.8

* nix-2.24.9

* lix-2.90.0

* lix-2.91.1

(I am also a lix user but tested and build in nix-shell -p nixVersions.nix_2_18 etc)

Spawning a shell is insufficient. The Nix daemon is unaffected by this.

Sorry, didn't think. I tested an fedora, my daemon is from nix-2.21.2 🤷‍♂️

Yeah can confirm for 2.18 now (back at my NixOS machine). And nice find. I just wonder why @chewblacka said the build worked for them without this fix.

Copy link
Contributor

@niklaskorz niklaskorz left a comment

Choose a reason for hiding this comment

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

Building this PR fails for me:

       > install: cannot stat '/nix/store/dxgqlaxsjydgxshh29f0qsz7jszlkdgs-ente-auth-4.0.2/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png': No such file or directory
       For full logs, run 'nix log /nix/store/xsw15jfkniv34zbvlqc8s0d0hcg7ldml-ente-auth-4.0.2.drv'.

Comment on lines +81 to 84
FAV=$out/app/ente-auth/data/flutter_assets/assets/icons/auth-icon.png
ICO=$out/share/icons

install -D $FAV $ICO/ente-auth.png
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's move this to a follow-up PR as it's irrelevant to this fix, so we can unbreak the build first.

Copy link
Contributor

@zi3m5f zi3m5f left a comment

Choose a reason for hiding this comment

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

This fixes the error introduced by #345404 for me 👍

@niklaskorz
Copy link
Contributor

niklaskorz commented Nov 1, 2024

Regarding my build failure, it looks like this PR branched off at the wrong point in history (before #345404 was merged), so building this PR's branch does not work. Rebasing it on top of current master works though.

@Aleksanaa
Copy link
Member

This is not needed, because ofborg will rebase itself to master before testing, so does nixpkgs-review

@niklaskorz
Copy link
Contributor

niklaskorz commented Nov 1, 2024

This is not needed, because ofborg will rebase itself to master before testing, so does nixpkgs-review

Yeah just wanted to clarify why it did not build for me at first :)
(I use nom build github:nixos/nixpkgs/<pr-commit>#ente-auth instead of nixpkgs-review)

@niklaskorz
Copy link
Contributor

@MrSom3body can you add this text to the PR description, as this PR is part of ZHF:

ZHF: #352882

@MrSom3body
Copy link
Contributor Author

Will do when I'm back home

@Aleksanaa Aleksanaa added the 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign label Nov 1, 2024
@wegank wegank added the 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people label Nov 1, 2024
@gepbird
Copy link
Contributor

gepbird commented Nov 1, 2024

I tested it with nix-build and nom build in my local nixpkgs on NixOS, I'm also using Lix from nixpkgs that is a few days behind nixos-unstable. Works an master, fails on this PR.

Edit: After looking at the flutter PR that broke this package, I assume this fix is good but just to be sure I'm building it (also a webkitgtk rebuild is needed).

@gepbird gepbird added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Nov 1, 2024
@wegank wegank removed the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Nov 1, 2024
@FliegendeWurst FliegendeWurst added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 1, 2024
@wegank wegank added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Nov 1, 2024
@wegank wegank merged commit 523318b into NixOS:master Nov 2, 2024
37 of 38 checks passed
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Builds on x86_64-linux, thanks!

@MrSom3body MrSom3body deleted the fix-ente-auth branch November 2, 2024 10:07
@niklaskorz niklaskorz mentioned this pull request Nov 2, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants