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-jlink: init at 794a, nrfconnect: init at 4.3.0, nrf-command-line-tools: init at 10.23.2 #255185

Merged
merged 3 commits into from
Feb 6, 2024

Conversation

StarGate01
Copy link
Member

@StarGate01 StarGate01 commented Sep 14, 2023

Description of changes

This PR re-adds some packages which were purged due to their usage of QT4 in #174634 : segger-jlink, and two packages which depend on it: nrfconnect and nrf-command-line-tools.

Unfortunately the SEGGER J-Link tools are only distributed in binary form and have a hard requirement on (the bundled) QT4. I have packaged the bundled QT4 libs as a secondary package to not pollute the global namespace.

Supersedes #252137, #244637, #263487

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.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.

@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 Sep 14, 2023
curlOpts = "--data accept_license_agreement=accepted";
};

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.

Why have this as a second derivation and not just as part of the main one?

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 wanted to ensure that QT4 libs (which are considered outdated) are not contained in /bin or /lib of a top-level package, in order to not pollute the global namespace. Am I mistaken? Should this be included?

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 that makes sense

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nrfconnect-package-wrongly-removed/33261/2

@Mindavi
Copy link
Contributor

Mindavi commented Oct 3, 2023

I think this may be more suitable as an overlay at this point

@pinpox
Copy link
Member

pinpox commented Oct 26, 2023

@StarGate01 Are you still working on this? I'm waiting for this package.

@pinpox pinpox mentioned this pull request Oct 26, 2023
12 tasks
@StarGate01
Copy link
Member Author

@pinpox Yes I am , I'll implement the suggestions made by @SuperSandro2000 , however I don't really agree with @Mindavi - I would want to keep this as a package instead of a (somewhat cumbersome) overlay.

@liarokapisv
Copy link
Contributor

liarokapisv commented Oct 27, 2023

The J-Links are an industry standard, I believe that the package is definitely worth keeping. I don't think that leaving the bundled qt4 libraries as is, is that huge of a deal in this case.

@StarGate01 StarGate01 changed the title segger-jlink: init at 792e, nrfconnect: init at 4.2.0, nrf-command-line-tools: init at 10.23.0 segger-jlink: init at 792m, nrfconnect: init at 4.2.1, nrf-command-line-tools: init at 10.23.2 Oct 27, 2023
@StarGate01
Copy link
Member Author

I have implemented most of the suggestions by @SuperSandro2000 , and also updated all three packages to the latest versions.

@liarokapisv
Copy link
Contributor

@StarGate01 I think it is useful to also provide desktop files for the graphical applications like JLinkRTTViewer, JMem etc.
I have a working reference implementation in my own pull-request (which I will probably close in favor of this one) if it is deemed useful.

@StarGate01
Copy link
Member Author

@liarokapisv Good idea - is the list at https://github.com/liarokapisv/nixpkgs/blob/1f6683d53373179b761055239616c7e65a0a71fc/pkgs/by-name/jl/jlink-software-tools/package.nix#L95 the complete list of all apps which should have a dektop icon?

@liarokapisv
Copy link
Contributor

I think so. It was taken from the AUR package. I double-checked it but I could be missing something.

@StarGate01
Copy link
Member Author

Okay. Some entries like JLink are CLI tools, would it be a sensible to add a dektop entry for those CLI-only tools?

@liarokapisv
Copy link
Contributor

liarokapisv commented Oct 27, 2023

I manually went through all executables and this is what I got.

GUIs:

  • JFlash
  • JFlashLite
  • JFlashSPI
  • JLinkConfig
  • JLinkGDBServer
  • JLinkLicenseManager
  • JLinkRTTViewer
  • JLinkRegistration
  • JLinkRemoteServer
  • JLinkSWOViewer
  • JLinkUSBWebServer
  • JMem
  • JScope

CLI-only:

  • JLink
  • JLinkGUIServer
  • JLinkRTTClient
  • JLinkRTTLogger
  • JLinkSTM32
  • JLinkXVCDServer
  • JRun
  • JTAGLoad

As a sidenote - the official deb package also makes various symlinks without the Exe suffix.
There are a few exceptions, most notably:

  • JLinkExe
  • JLinkConfigExe
  • JLinkGDBServerCLExe
  • JLinkGUIServerExe
  • JLinkRTTViewerExe
  • JLinkSWOViewerCLExe
  • JLinkUSBWebServerExe
  • JLinkXVCDServerExe

These do not have symlinks without the Exe suffix.
Maybe it would be a good idea to provide symlinks for these too?

maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
maxhbr

This comment was marked as outdated.

maxhbr added a commit to maxhbr/zephyr-flake that referenced this pull request Jan 18, 2024
Signed-off-by: Maximilian Huber <[email protected]>
Copy link
Member

@maxhbr maxhbr left a comment

Choose a reason for hiding this comment

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

Works for me 👍

@teburd
Copy link
Contributor

teburd commented Feb 1, 2024

Works for me, and I'm using it in a flake today with an overlay in a very janky and unfriendly way. Would really much prefer this in tree

@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-ready-for-review/3032/3395

@Mindavi Mindavi merged commit 06de1cf into NixOS:master Feb 6, 2024
18 checks passed
Copy link
Contributor

github-actions bot commented Feb 6, 2024

@Mindavi
Copy link
Contributor

Mindavi commented Feb 6, 2024

Okay, there seems to be a need for this, and hydra won't help building this anyway. So I guess we can carry this package for now. But I'm not super thrilled about carrying such an old qt, it'll inevitably break at some point, with a compiler update or so. Anyhow, it won't burden hydra much.

@h7x4
Copy link
Member

h7x4 commented Feb 7, 2024

Maybe it would be possible to split the package? I suspect that most things depending on segger jlink only depends on the libraries and/or cli tools, not the GUI stuff. Some kind of segger-jlink-headless?

@Mindavi
Copy link
Contributor

Mindavi commented Feb 7, 2024

QT also contains quite some libraries, it seems unlikely only the GUI parts were used and none of the non-gui parts.

@StarGate01
Copy link
Member Author

Interesting idea, we would have to figure out which of the tools actually use QT. This might also change with future updates. I am unsure if there is a usecase for a "reduced" JLink tool suite - even mundane things like a license popup use QT4.

@emilazy
Copy link
Member

emilazy commented Nov 5, 2024

I’ve opened #353880 to revert this, per #214195 (comment).

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.