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

Add the system-monitor applet #626

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from
Draft

Conversation

ziguana
Copy link

@ziguana ziguana commented Jan 28, 2025

Closes #625

TODO:

Copy link
Owner

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

Thank you so much for this contribution! There's a few minor nits, but otherwise this looks great :)

resolve the duplicate dependency issue

This will be fixed in Nixpkgs via NixOS/nixpkgs#282798 but you can also bug upstream to add a line like this to their Cargo.toml to fix it: https://github.com/cosmic-utils/tasks/blob/406ffac6a2263cdc1006f3578b3f9be64761d6ea/Cargo.toml#L50-L51

pkgs/cosmic-ext-applet-system-monitor/Cargo.lock Outdated Show resolved Hide resolved
pkgs/cosmic-ext-applet-system-monitor/package.nix Outdated Show resolved Hide resolved
pkgs/cosmic-ext-applet-system-monitor/package.nix Outdated Show resolved Hide resolved
pkgs/cosmic-ext-applet-system-monitor/package.nix Outdated Show resolved Hide resolved
pkgs/cosmic-ext-applet-system-monitor/package.nix Outdated Show resolved Hide resolved
@ziguana
Copy link
Author

ziguana commented Jan 31, 2025

Thanks for the review! Sorry for blindly copying some useless stuff from another applet. I pushed a non-working WIP branch, as I still don't understand where to get Cargo.lock from.

@ziguana ziguana force-pushed the main branch 2 times, most recently from f0e7534 to 4ef5ec8 Compare February 4, 2025 01:11
@lilyinstarlight
Copy link
Owner

Thanks for the review! Sorry for blindly copying some useless stuff from another applet. I pushed a non-working WIP branch, as I still don't understand where to get Cargo.lock from.

No worries, I get it :)

Do you think you could bug upstream to add those two lines to Cargo.toml like https://github.com/cosmic-utils/tasks/blob/406ffac6a2263cdc1006f3578b3f9be64761d6ea/Cargo.toml#L50-L51, to fix the cargo vendor bug so we can get this in?

@ziguana
Copy link
Author

ziguana commented Feb 9, 2025

I've been coming back to this now and again, and I can't make it work. Could be a skill issue.

My approach has been to try and remove these patch lines, which upstream is obviously not going to accept. However, patching it locally before the build doesn't seem to work. Looking at fetch-cargo-vendor-util.py, this must be because it's parsing Cargo.lock directly.

What do you think is the right approach here?

@ziguana
Copy link
Author

ziguana commented Feb 9, 2025

Forgot to add that patching Cargo.lock doesn't seem to work either.

@lilyinstarlight
Copy link
Owner

lilyinstarlight commented Feb 9, 2025

What do you think is the right approach here?

I guess to be clear, I've been saying to copy those two lines https://github.com/cosmic-utils/tasks/blob/406ffac6a2263cdc1006f3578b3f9be64761d6ea/Cargo.toml#L50-L51 verbatim into https://github.com/D-Brox/cosmic-ext-applet-system-monitor/blob/f29debf726551df521aa26070c8efc54d7c0a714/Cargo.toml#L31, update Cargo.lock with cargo metadata command or something, and then PR the diff upstream

Does that not work?

@ziguana
Copy link
Author

ziguana commented Feb 9, 2025

Yeah, I tried that first, but thought that patch didn't apply transitively. I pushed the override in case you wanted to take a look. Maybe I'm doing something wrong.

@lilyinstarlight
Copy link
Owner

lilyinstarlight commented Feb 9, 2025

I pushed the override in case you wanted to take a look. Maybe I'm doing something wrong.

I pulled from your branch and updated cargoHash (since it didn't look like you updated it when updating src revision) and it seems to fetch cargo deps just fine. When attempting to build, it did also tell me I had to add pkg-config to nativeBuildInputs and fontconfig to buildInputs. After though, it seemed to build just fine too

Does updating that stuff mentioned above work, or if not what error are you getting?

@ziguana
Copy link
Author

ziguana commented Feb 9, 2025

Oh, y..yeah, it was cargoHash. My bad.

I pushed a version that works, but it depends on my fork of upstream. I really don't think upstream should care about pinning sctk for us, but I was not able to successfully override Cargo.lock with a patch.

@lilyinstarlight
Copy link
Owner

I really don't think upstream should care about pinning sctk for us

It's not really "for us". Most distros would need it too, since even the cargo vendor command would not work as-is (upstream COSMIC repos and cosmic-utils repos do these patch statements for the same reasons -- their own tooling doesn't even work if cargo vendor doesn't)

Patch for Cargo.lock should work regardless though, so I'll poke at that later

@ziguana
Copy link
Author

ziguana commented Feb 9, 2025

I just needed to use cargoPatches instead of patches. I pushed the version with patched (instead of forked) upstream. I propose carrying this patch until the issue is fixed in nixpkgs, but it's your call.

Going to make sure system-monitor works next.

@ziguana
Copy link
Author

ziguana commented Feb 9, 2025

It's not in the output of cosmic-applets, going to look for a way to add it next.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Package cosmic-ext-applet-system-monitor.
3 participants