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

segger-systemview: init at 352a #214195

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StarGate01
Copy link
Member

@StarGate01 StarGate01 commented Feb 2, 2023

Description of changes

This PR adds the SEGGER SystemView debugging utility. It depends on the SEGGER J-Link libraries which are tracked in PR #255185 .

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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 2, 2023
@StarGate01 StarGate01 force-pushed the segger-systemview branch 8 times, most recently from af37caf to fb3b0f0 Compare September 14, 2023 16:44
@StarGate01 StarGate01 force-pushed the segger-systemview branch 5 times, most recently from 17094a5 to 90ac35f Compare October 28, 2023 11:17
@StarGate01 StarGate01 force-pushed the segger-systemview branch 4 times, most recently from 490ba94 to eebe8eb Compare December 11, 2023 02:05
@StarGate01 StarGate01 force-pushed the segger-systemview branch 3 times, most recently from 29d6407 to 5666700 Compare December 21, 2023 11:23
@StarGate01
Copy link
Member Author

Now that #255185 has been merged, I have rebased this PR to only contain the changes for the systemview package.

pkgs/by-name/se/segger-systemview/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/se/segger-systemview/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/se/segger-systemview/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/se/segger-systemview/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/se/segger-systemview/package.nix Outdated Show resolved Hide resolved
@StarGate01 StarGate01 changed the title segger-systemview: init at 3.42 segger-systemview: init at 3.52a Feb 23, 2024
@StarGate01 StarGate01 changed the title segger-systemview: init at 3.52a segger-systemview: init at 352a Feb 23, 2024
@StarGate01 StarGate01 requested a review from h7x4 February 26, 2024 10:03
@StarGate01 StarGate01 force-pushed the segger-systemview branch from 7a868b3 to 1ae55ed Compare May 10, 2024 18:37
@StarGate01 StarGate01 force-pushed the segger-systemview branch from 1ae55ed to f3652b8 Compare May 21, 2024 14:55
pkgs/by-name/se/segger-systemview/package.nix Outdated Show resolved Hide resolved
inherit (platform) sha256;
};

qt4-bundled = stdenv.mkDerivation {
Copy link
Member

Choose a reason for hiding this comment

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

We don't accept more qt4 package and have been trying to remove or update them.

Copy link
Member Author

Choose a reason for hiding this comment

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

The package has a hard dependency on QT4. Without QT4, it does not run. Please review the discussion on QT4 at #255185 , SEGGER is unwilling to upgrade the QT version.

Copy link
Member

Choose a reason for hiding this comment

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

Please see #255185 and #319342 for context.

We know that the segger software reintroduce qt4 dependencies, but the dependency is not exposed to toplevel, and the package is properly marked as insecure. The user knows what they are doing if they install this package.

Copy link
Member

Choose a reason for hiding this comment

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

So to introduce a proprietary software with outdated dependencies... why? It's not like it was ever there and we need to remove it.

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 understand your concerns, but as @h7x4 pointed out, this package required the user to explicitly understand and allow (1) the license and (2) the outdated libs. Other packages and systems are unaffected by this package, i.e. if you do not use it you are unaffected.

SystemView is a widely used industry-standard embedded debugging suite, see https://www.segger.com/products/development-tools/systemview/ . It would be nice for NixOS to evolve from a project for ambitions hobbyists into a system which supports real-world software.

If I could, I would just provide this package out-of tree, similar to how e.g. Arch (https://aur.archlinux.org/packages/jlink-systemview) does it, but this does not work for nixpkgs. I do not have the bandwidth to maintain an eternal fork for a simple package.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

We might be able to, but the bundled QT libs may change in between releases. AFAIK only the libraries which are directly needed by the related package are included, which might not be enough for the other packages.

I have not verified this, but I am hesitant to de-duplicate these "independent" libs.

This comment was marked as duplicate.

@emilazy
Copy link
Member

emilazy commented Nov 5, 2024

Hi, sorry that this PR was dormant for a long time.

If I could, I would just provide this package out-of tree, similar to how e.g. Arch (https://aur.archlinux.org/packages/jlink-systemview) does it, but this does not work for nixpkgs. I do not have the bandwidth to maintain an eternal fork for a simple package.

This is totally possible – since this is a leaf package that other packages don’t depend on, you can put up a repository containing just the additional packages that users can overlay on top of Nixpkgs. Flakes can help do this, and there are already a lot of third‐party repositories doing this (e.g. rust-overlay, nixos-cosmic, and even from before flakes were a thing the Nix User Repository).

I would encourage you to do that since we removed Qt 4 two and a half years ago and really don’t want to accept new packages using Qt 4 or take up the burden of maintaining them; if anything, we’re trying to reduce out use of Qt 5 in anticipation of it going EOL next year. I think that #255185 should not have been accepted either, unfortunately, but it should be simple to provide these packages in your own repository without having to maintain a fork of Nixpkgs. Please let me know if you need any pointers on how to accomplish this.

emilazy added a commit to emilazy/nixpkgs that referenced this pull request Nov 5, 2024
This is a proprietary package that depends on the
vulnerability‐ridden Qt 4, and was removed along with Qt 4 in
2022. It was re‐added at the beginning of the year, but upstream
is unwilling to upgrade their Qt version and as it is proprietary
software we have no way to patch it on our end.

I think that it would be best not to ship this in 24.11. While it’s
clearly useful software, we made the deliberate decision to remove Qt
4 seven years after it reached end of life, and since this software
is seemingly to eternal `knownVulnerabilities` I think that it does
not meet our standards for inclusion. This would be better maintained
in a third‐party repository, as suggested by the maintainer in
<NixOS#214195 (comment)>.

This reverts commit d5fd263.
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Nov 5, 2024
This is a proprietary package that depends on the
vulnerability‐ridden Qt 4, and was removed along with Qt 4 in
2022. It was re‐added at the beginning of the year, but upstream
is unwilling to upgrade their Qt version and as it is proprietary
software we have no way to patch it on our end.

I think that it would be best not to ship this in 24.11. While it’s
clearly useful software, we made the deliberate decision to remove Qt
4 seven years after it reached end of life, and since this software
is seemingly to eternal `knownVulnerabilities` I think that it does
not meet our standards for inclusion. This would be better maintained
in a third‐party repository, as suggested by the maintainer in
<NixOS#214195 (comment)>.

This reverts commit d5fd263.
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Nov 5, 2024
This is a proprietary package that depends on the
vulnerability‐ridden Qt 4, and was removed along with Qt 4 in
2022. It was re‐added at the beginning of the year, but upstream
is unwilling to upgrade their Qt version and as it is proprietary
software we have no way to patch it on our end.

I think that it would be best not to ship this in 24.11. While it’s
clearly useful software, we made the deliberate decision to remove Qt 4
seven years after it reached end of life, and since this software is
seemingly doomed to eternal `knownVulnerabilities` I think that it does
not meet our standards for inclusion. This would be better maintained in
a third‐party repository, as suggested by the maintainer in
<NixOS#214195 (comment)>.

This reverts commit d5fd263.
emilazy added a commit to emilazy/nixpkgs that referenced this pull request Nov 5, 2024
This is a proprietary package that depends on the
vulnerability‐ridden Qt 4, and was removed along with Qt 4 in
2022. It was re‐added at the beginning of the year, but upstream
is unwilling to upgrade their Qt version and as it is proprietary
software we have no way to patch it on our end.

I think that it would be best not to ship this in 24.11. While it’s
clearly useful software, we made the deliberate decision to remove Qt 4
seven years after it reached end of life, and since this software is
seemingly doomed to eternal `knownVulnerabilities` I think that it does
not meet our standards for inclusion. This would be better maintained in
a third‐party repository, as suggested by the maintainer in
<NixOS#214195 (comment)>.

This reverts commit d5fd263.
@StarGate01
Copy link
Member Author

@emilazy Thank you for your suggestion. Although I don't agree on the removal of the tools from nixpkgs (yet), I would like to explore the option of providing them via a flake. Do you have some resources I could use as a template?

@emilazy
Copy link
Member

emilazy commented Nov 5, 2024

The simplest thing would be a flake that just provides an overlay:

{
  outputs = { self }: {
    overlays.default = final: prev: {
      segger-jlink = final.callPackage ./pkgs/segger-jlink { };
      nrfconnect = final.callPackage ./pkgs/nrfconnect { };
      # …
    };
  };
}

Users could include it in their configuration with inputs.segger-flake.url = "…"; and set nixpkgs.overlays = [ inputs.segger-flake.overlays.default ]; to get the packages available. If you wanted to expose a full package set so that things like nix run …#segger-jlink also worked, you could do:

{
  inputs = {
    nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
  };

  outputs = { self, nixpkgs }: {
    packages = let
      systems = [
        "x86_64-linux"
        "i686-linux"
        "aarch64-linux"
        "armv7l-linux"
      ];
    in nixpkgs.lib.genAttrs systems (system:
      nixpkgs.legacyPackages.${system}.extend self.overlays.default
    );

    overlays.default =; # same as before
  };
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants