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

cosmic-term: init at unstable-2023-12-26 #276959

Merged
merged 1 commit into from
Jan 3, 2024

Conversation

ahoneybun
Copy link
Contributor

@ahoneybun ahoneybun commented Dec 26, 2023

Description of changes

COSMIC Term is the Terminal Emulator for the COSMIC DE:

https://github.com/pop-os/cosmic-term

Screenshot from 2023-12-28 11-52-13

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/)
  • 23.11 Release Notes (or backporting 23.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.

postInstall = ''
wrapProgram "$out/bin/${pname}" \
--suffix XDG_DATA_DIRS : "${cosmic-icons}/share" \
--prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ xorg.libX11 ]}
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll need to add two extra dependencies to LD_LIBRARY_PATH because winit dlopens them on wayland:

Suggested change
--prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ xorg.libX11 ]}
--prefix LD_LIBRARY_PATH : ${lib.makeLibraryPath [ xorg.libX11 wayland libxkbcommon ]}

BTW, since cosmic-epoch is wayland-only, do we still need to keep X11?

Copy link

@Nexushunter Nexushunter Dec 26, 2023

Choose a reason for hiding this comment

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

I agree, X11 should be dropped where possible or XWayland should be used if X11 compatibility is needed 🤔 (I wasn't aware that it was Wayland only)

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 builds without them is there a reason to add them?

The X11 packages will need to stay there to keep it working on Xorg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, X11 should be dropped where possible or XWayland should be used if X11 compatibility is needed 🤔 (I wasn't aware that it was Wayland only)

By wayland only, I mean that there's no X11 session anymore -- the session is wayland only. Xwayland still exists to keep backward compatibility.

It builds without them is there a reason to add them?

It will give a NoWaylandLib error if you unset DISPLAY, since it tries to use dlopen to open the wayland library, which is not existent on NixOS.

To be able to find the library, you'll either to force linking like this:

https://github.com/NixOS/nixpkgs/blob/d1fcabefe1617c4dd295774692140b2018b9f9fc/pkgs/by-name/co/cosmic-panel/package.nix#L47C2-L53C7

or wrap with LD_LIBRARY_PATH including wayland.

The X11 packages will need to stay there to keep it working on Xorg.

If you're using cosmic-term with cosmic-session, then there's no reason to keep X11. Although one could argue that they want to use cosmic tools on some other X11 DE, in which case yes the package needs to be kept.

Copy link
Contributor Author

@ahoneybun ahoneybun Dec 26, 2023

Choose a reason for hiding this comment

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

This is meant to go with cosmic-session so you would have DISPLAY set so I believe it is best to have it set.

EDIT: I see the issue if we don't set that. I figured it is fine since I have it like that with cosmic-edit so I didn't understand the issue to not have it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I unset DISPLAY simply to test if it works under wayland. Currently without adding wayland and libxkbcommon to the LD_LIBRARY_PATH it will refuse to run under wayland at all. It's not ideal to have to run through Xwayland for an application that is designed to work in a Wayland-only DE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wayland of cosmic-edit works because it links directly with libwayland-egl instead of opening it via dlopen. That's not true for cosmic-term.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it and it seems to work on X11/Xorg for me still. Does it have any issues on Wayland?

@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Dec 26, 2023
Copy link
Contributor

@nbdd0121 nbdd0121 left a comment

Choose a reason for hiding this comment

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

LGTM, I think this depends on #276072?

@ahoneybun
Copy link
Contributor Author

Yes it does. I left it as a draft mainly due to that.

@ahoneybun ahoneybun marked this pull request as ready for review January 2, 2024 14:44
@ahoneybun
Copy link
Contributor Author

Since #276072 was merged this is now ready for review!

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Jan 2, 2024
Copy link
Member

@cole-h cole-h left a comment

Choose a reason for hiding this comment

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

LGTM, builds and runs fine in a not-COSMIC environment.

@cole-h
Copy link
Member

cole-h commented Jan 3, 2024

(ofborg build failure is because this branch doesn't have #276072, but checking this branch out and manually rebasing it on master builds successfully)

@cole-h cole-h merged commit a196989 into NixOS:master Jan 3, 2024
25 checks passed
@ahoneybun ahoneybun deleted the init-cosmic-term branch January 3, 2024 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants