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

wmenu: init at 0.1.2 #218300

Merged
merged 2 commits into from
Apr 1, 2023
Merged

wmenu: init at 0.1.2 #218300

merged 2 commits into from
Apr 1, 2023

Conversation

Eken-beep
Copy link
Contributor

@Eken-beep Eken-beep commented Feb 25, 2023

Description of changes

add wmenu as requested by #217862

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, 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/)
  • 23.05 Release Notes (or backporting 22.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.

@kirillrdy
Copy link
Member

@Eken-beep

Hi, thank you for your PR

for me however this derivation doesnt build

error: builder for '/nix/store/gljq62cddal8n00z5fjcb6fqkljg0dbl-wmenu-0.1.2.drv' failed with exit code 1;
       last 10 log lines:
       > [10/12] Compiling C object wmenu.p/pool-buffer.c.o
       > [11/12] Compiling C object wmenu.p/main.c.o
       > FAILED: wmenu.p/main.c.o
       > gcc -Iwmenu.p -I. -I.. -Iprotocols -I/nix/store/mvhiwg7wkacdmi6p6j1sb3sm4ww3mq6z-cairo-1.16.0-dev/include/cairo -I/nix/store/ag35f2gsrl7j8paxdcv2jhr6h35h2n10-freetype-2.12.1-dev/include/freetype2 -I/nix/store/ag35f2gsrl7j8paxdcv2jhr6h35h2n10-freetype-2.12.1-dev/include -I/nix/store/qxfdxjwd1n8izbn5csg6bm925zfzibhv-glib-2.74.3-dev/include -I/nix/store/qxfdxjwd1n8izbn5csg6bm925zfzibhv-glib-2.74.3-dev/include/glib-2.0 -I/nix/store/jzh15bi6zablx3d9s928w3lgqy6and91-glib-2.74.3/lib/glib-2.0/include -I/nix/store/vffghgqfw7md3jg7qclnbfy2afzri65l-pango-1.50.12-dev/include/pango-1.0 -I/nix/store/bd3x5n8mrmhz65c81xi8m669kixhgp7v-harfbuzz-6.0.0-dev/include/harfbuzz -I/nix/store/g0lx8manh822bg118ycj9wyzz8l1fdxn-wayland-1.21.0-dev/include -I/nix/store/xdp0mvpqydy5b81xhfi4hq51zw4sx0z1-libxkbcommon-1.5.0-dev/include -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wextra -Werror -std=c11 '-DVERSION="0.1.2"' -Wno-missing-field-initializers -Wno-unused-parameter -Wundef -Wvla -MD -MQ wmenu.p/main.c.o -MF wmenu.p/main.c.o.d -o wmenu.p/main.c.o -c ../main.c
       > ../main.c: In function 'keypress':
       > ../main.c:761:17: error: '__builtin_strncpy' specified bound 8192 equals destination size [-Werror=stringop-truncation]
       >   761 |                 strncpy(state->text, state->selection->text, sizeof state->text);
       >       |                 ^
       > cc1: all warnings being treated as errors
       > ninja: build stopped: subcommand failed.
       For full logs, run 'nix log /nix/store/gljq62cddal8n00z5fjcb6fqkljg0dbl-wmenu-0.1.2.drv'.
error: 1 dependencies of derivation '/nix/store/759dapkqa9b71ijk1bi44dp3wma7krgw-review-shell.drv' failed to build

if this PR is still work in progress please mark it as draft,

other feedback is commits and commit messages need to meet contribution guidelines, outlines in the CONTRIBUTING.md.

@kirillrdy
Copy link
Member

also there is traling white space which fails ofborg EditorConfig check

@Eken-beep
Copy link
Contributor Author

It seems as though the issue lies with wmenu as i can't solve this, should i still make it a draft?

@kirillrdy
Copy link
Member

@Eken-beep Sorry I now properly read your comment about that it fails to compile

@kirillrdy
Copy link
Member

@Eken-beep

+  env.NIX_CFLAGS_COMPILE = toString [
+    "-Wno-error=stringop-truncation"
+  ];
+

you can disable that warning to get the build working, although it almost looks like a bug that should be reported upstream.

once it builds are you able to test it ? eg run it

@Eken-beep
Copy link
Contributor Author

@kirillrdy
Because the Nvidia driver is currently broken i can't test it but I'll try in a VM later today.

@Eken-beep
Copy link
Contributor Author

Eken-beep commented Feb 26, 2023

Ok the building of the binary works for me too after disabling warnings but it crashes instantly with a similar error message as the warning;

wmenu: ../main.c:1019: menu_init: Assertion `state->layer_shell != NULL' failed.
fish: Job 1, './result/bin/wmenu' terminated by signal SIGABRT (Abort)

I'll report this issue to wmenu as for now, should i mark the package as broken and proceed or something?

@Eken-beep
Copy link
Contributor Author

The guy working on wmenu fixed the warning now and the binary both compiles and works.

@Eken-beep Eken-beep requested a review from kirillrdy March 1, 2023 14:44
@kirillrdy
Copy link
Member

@Eken-beep

  • wmenu builds for me but crashes, looks like we need to apply the patch from upstream or wait for next release
  • squash commits and change commit message to comply with contribution guidelines CONTRIBUTING.md.

@Eken-beep Eken-beep changed the title Add wmenu wmenu: init at 0.1.2 Mar 2, 2023
Copy link
Member

@kirillrdy kirillrdy left a comment

Choose a reason for hiding this comment

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

  • still crashes for me, although i ran it from inside gnome wayland, maybe it works in sway
  • need to either add yourself as maintainer or find a maintainer
  • this PR should consist of just 2 commits, first with message like maintainers: add yourusername and second wmenu: init at 0.1.2

pkgs/applications/misc/wmenu/default.nix Show resolved Hide resolved
pkgs/applications/misc/wmenu/default.nix Show resolved Hide resolved
platforms = platforms.linux;
};

env.NIX_CFLAGS_COMPILE = toString [
Copy link
Member

Choose a reason for hiding this comment

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

after applying patch this is no longer needed

@Eken-beep
Copy link
Contributor Author

Apparently gnome doesn't support gtk layer shell, i fell for that too. I got it working in hyprland though. The patch only removes the warning.

@kirillrdy
Copy link
Member

@Eken-beep currently this PR contains 15 commits. Can you please squash so that its only 2 commits

the reason for squash is to have commits that evaluate so that it can be used for automated git bisect to find any regressions.

I can also see that you have a merge with your own branch ( commits appear twice )

maybe simplest way is to do

git reset 06365e2b599e852409efa892beff6f22681c1d8d # this is where you started your work

then git add maintainers list as first commit
and changes to all-packages.nix and and wmenu/default.nix as second commit

after that I think if this works for you we should be good

@Eken-beep Eken-beep force-pushed the master branch 2 times, most recently from 3ff1520 to 06365e2 Compare March 4, 2023 12:13
@Eken-beep
Copy link
Contributor Author

Thank you for taking your time helping me, this seems to be what CONTRIBUTING.md is after.

@Eken-beep
Copy link
Contributor Author

Yes, cmake was unneccesary, don't know why that got there.

@kirillrdy
Copy link
Member

@Eken-beep in your recent commits you are trying to delete pkgs/applications/networking/cluster/argocd-autopilot/default.nix

if this gets merged then that file will be removed from reposiroy, but its still rerefenced in all-packages.nix thats why ofborg-eval failed

there are many ways of fixing this, here is one of them

git rebase -i ef2a78723ea69ebf2b59a7f4602f5e0cf7d00db3

this will open text editor $EDITOR

in there,we want to remove commits that introduce changes to argocd-autopilot, and we want to sqash you last commit that removes cmake

result looks like

pick 55ee253d09b maintainers: eken
pick 3dad25c5bbf wmenu: init at 0.1.2
squash 9194a98af73 Update default.nix

after that you should be left with just 2 commits and changes that only related to wmenu and you being maintainer

@Eken-beep
Copy link
Contributor Author

I'm sorry but I tried fixing it with the GitHub web editor and broke it I'll try later.

, fetchFromSourcehut
, fetchpatch
, pkg-config
, cmake
Copy link
Member

Choose a reason for hiding this comment

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

cmake is not used

pkgs/applications/misc/wmenu/default.nix Show resolved Hide resolved
@Eken-beep Eken-beep force-pushed the master branch 3 times, most recently from 016aa0d to acfed1c Compare March 12, 2023 21:18
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/861

})
];

nativeBuildInputs = [ pkg-config meson ninja wayland-protocols scdoc ];
Copy link
Member

Choose a reason for hiding this comment

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

sorry for duplicated comment:

with strictDeps = true and wayland-protocols scdoc in nativeBuildInputs this derivation no longer builds

should we undo ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, remove strictDeps and keep wayland-protocols and scdoc in nativeBuildInputs. I haven't tried with strictDeps on.

Copy link
Member

Choose a reason for hiding this comment

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

i think intention for strictDeps to be on by default #178468

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Breaks for me too with strictDeps on... Should this be reverted, I mean it worked fine with everything in buildInputs.

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