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

Jump to bottom

itm-tools: init at 2019-11-15 #91552

Merged
merged 2 commits into from Aug 24, 2020
Merged

itm-tools: init at 2019-11-15 #91552

merged 2 commits into from Aug 24, 2020

Conversation

HarryMakes
Copy link
Contributor

@HarryMakes HarryMakes commented Jun 26, 2020 •
edited

Motivation for this change

Update: As per discussion on rust-embedded/itm#24 (comment), the recommended itm-tools will be provided by this proposed Nix package instead. It will install the following binaries for the user:

  • port-demux: ITM Port Demuxing, which share similar functions as itmdump
  • excevt: Exception Tracing
  • pcsampl: Program Counter Sampling

This tool is available as a binary on crates.io (link here), but its source code and Cargo.lock need to be patched in order to be compiled with Rust >=1.41.0 (corresponding to nixpkgs 20.03).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from sbourdeauducq June 26, 2020 07:22
sbourdeauducq
sbourdeauducq reviewed Jun 26, 2020
View reviewed changes
pkgs/development/tools/misc/itmdump/default.nix Outdated
@@ -0,0 +1,28 @@
{ stdenv, fetchFromGitHub, rustPlatform, pkgconfig }:
Copy link
Contributor

Choose a reason for hiding this comment

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

#89206 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

sbourdeauducq
sbourdeauducq reviewed Jun 26, 2020
View reviewed changes
pkgs/development/tools/misc/itmdump/itm-src.patch Outdated
}

-fn open_read(matches: &ArgMatches) -> Result<Box<io::Read + 'static>, io::Error> {
+fn open_read(matches: &ArgMatches) -> Result<Box<dyn io::Read + 'static>, io::Error> {
Copy link
Contributor

Choose a reason for hiding this comment

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

I still do not understand why cargo install itm inside nix-shell -p cargo on nixos 20.03.2310.fb6c3a6831c completes successfully without this patch.

@ofborg ofborg bot requested a review from sbourdeauducq June 26, 2020 08:04
danieldk
danieldk suggested changes Jul 6, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk 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 for your first contribution 👍 and sorry for having to wait so long for a review. I have added an initial set of comments.

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/development/tools/misc/itmdump/default.nix Outdated
Comment on lines 4 to 5
version = "0.3.1";
pname = "itm";
Copy link
Contributor

Choose a reason for hiding this comment

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

Swap order (since it's more canonical).

Suggested change
version = "0.3.1";
pname = "itm";
pname = "itm";
version = "0.3.1";

pkgs/development/tools/misc/itmdump/default.nix Outdated

src = fetchFromGitHub {
owner = "rust-embedded";
repo = "itm";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
repo = "itm";
repo = pname;

pkgs/development/tools/misc/itmdump/default.nix Outdated
description = "Tool to parse and dump ITM packets";
homepage = "https://github.com/rust-embedded/itm";
license = with licenses; [ asl20 mit ];
maintainers = with maintainers; [ hh sb0 ];
Copy link
Contributor

Choose a reason for hiding this comment

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

@sbourdeauducq do you want to be a maintainer?

pkgs/development/tools/misc/itmdump/default.nix Outdated
Comment on lines 8 to 9
owner = "rust-embedded";
repo = "itm";
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that itmdump was removed from that repository and has moved to:

https://github.com/japaric/itm-tools

So this derivation should probably use that instead (there are no releases yet, so the version could be date-based). The attribute name and derivation directory could then be renamed to itm-tools.

@HarryMakes
Copy link
Contributor Author

HarryMakes commented Jul 7, 2020 •
edited

Thank you for the feedback! I found a reference regarding such migration over here: rust-embedded/itm#24 (comment)

Other than renaming my package and the title of this pull request, can I keep my forked branch name as-is or must I rename the branch as well?

Edit: While itm-tools is the currently-supported version, it appears that there is no official release of it yet and hasn't been updated for more than half a year. itmdump however has an official latest release of v0.3.1 that is still listed on crates.io, and its non-itmdump/binary version has been updated only a few days ago. While I can contact @japaric about the situation, would it be more appropriate to keep itmdump on nixpkgs?

@danieldk
Copy link
Contributor

danieldk commented Jul 7, 2020

While I can contact @japaric about the situation, would it be more appropriate to keep itmdump on nixpkgs?

I think contacting them is the best recourse. Upstream authors are often willing to tag a release to make downstream packaging easier.

@HarryMakes HarryMakes changed the title itmdump: init at 0.3.1 itm-tools: init at 2019-11-15 Aug 21, 2020
@HarryMakes
Copy link
Contributor Author

HarryMakes commented Aug 21, 2020 •
edited

Just force pushed my branch to use the master HEAD of japaric/itm-tools instead. I didn't rename this branch, should it be fine?

danieldk
danieldk approved these changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

Result of nixpkgs-review pr 91552 1

1 package built:
- itm-tools

Tested binaries.

danieldk
danieldk suggested changes Aug 21, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Whoops, missed one minor issue. We mark date versions using unstable.

pkgs/development/tools/misc/itm-tools/default.nix Outdated

rustPlatform.buildRustPackage rec {
pname = "itm-tools";
version = "2019-11-15";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
version = "2019-11-15";
version = "unstable-2019-11-15";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in f437a1886583328c23bee9cd0f9b654378b4e07d

Copy link
Contributor

@danieldk danieldk Aug 24, 2020 •
edited

Choose a reason for hiding this comment

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

You need to change the hash as well:

hash mismatch in fixed-output derivation '/nix/store/45ikydicdgyi3d3w6zk26mj6p3dpi9bk-itm-tools-unstable-2019-11-15-vendor.tar.gz':
  wanted: sha256:1i2zfn8pk7v2cbws9iallc283pmfn3bqdqvp6cqj19hvkrxxs9p2
  got:    sha256:0rl2ph5igwjl7rwpwcf6afnxly5av7cd6va6wn82lxm606giyq75

BTW. if you keep the allow edits from maintainers option enabled, we do such minor fixes ourselves ;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks... but I can't find the option (checkbox) for allowing edits from maintainers now. Not sure how.

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot from 2020-08-24 08-40-08

See on the right.

danieldk
danieldk approved these changes Aug 24, 2020
View reviewed changes
Copy link
Contributor

@danieldk danieldk left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 91552 1

1 package built:
- itm-tools

Thanks a lot!

@danieldk danieldk merged commit f29f07d into NixOS:master Aug 24, 2020
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/how-exactly-is-cargosha256-computed/10727/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danieldk danieldk danieldk approved these changes

@sbourdeauducq sbourdeauducq Awaiting requested review from sbourdeauducq

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@HarryMakes @danieldk @nixos-discourse @sbourdeauducq @Janik-Haag