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

tokei: 9.1.1 -> 10.0.1 #68403

Merged
merged 1 commit into from Sep 11, 2019
Merged

tokei: 9.1.1 -> 10.0.1 #68403

merged 1 commit into from Sep 11, 2019

Conversation

lilyball
Copy link
Member

@lilyball lilyball commented Sep 10, 2019 •
edited

Enable all crate features, which gets us alternative output formats.

Update description and add long description to match what's in the GitHub repo.

Add myself as a maintainer.

Motivation for this change

The package was out of date and I wanted JSON output.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

I built this with no feature flags, with all feature flags, and with just includesJSONOutput and tested that the supported output formats for each build were as expected.

@ofborg ofborg bot requested a review from gebner September 10, 2019 06:20
gebner
gebner reviewed Sep 10, 2019
View reviewed changes
pkgs/development/tools/misc/tokei/default.nix Outdated

# feature flags
# the nixpkg package set looks up values for these flags from `config.tokei`.
, includeJSONOutput ? false
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to make these features optional? Typically we build packages with all features enabled by default.

Copy link
Member Author

Choose a reason for hiding this comment

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

They're not default on tokei's end. I don't really know why you wouldn't want them; they do introduce new dependencies. I did notice serde_json is in the dependency tree even without the feature on, though I don't know if it's the same version or if the feature compiles it twice.

I guess I should do a compilation time comparison between the two, and unless it's atrocious, then I can remove the flags and just default them to on because these don't pull in external dependencies or anything.

Copy link
Member Author

Choose a reason for hiding this comment

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

Release build with all the features takes about 14% more time, in my quick test (building directly rather than through Nix, but w/e). And that's only 2m02s on this machine. I'll just enable them all.

Copy link
Member Author

Choose a reason for hiding this comment

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

PR updated to remove these flags and just enable all features.

@lheckemann lheckemann added this to the 20.03 milestone Sep 10, 2019
@lilyball
tokei: 9.1.1 -> 10.0.1
0f34860 
Enable all crate features, which gets us alternative output formats.

Update description and add long description to match what's in the
GitHub repo.

Add myself as a maintainer.
@ofborg ofborg bot requested a review from gebner September 10, 2019 21:06
@gebner
Copy link
Member

gebner commented Sep 10, 2019

@GrahamcOfBorg build tokei

@gebner gebner merged commit 129fddf into NixOS:master Sep 11, 2019
@lilyball lilyball deleted the tokei branch September 11, 2019 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gebner gebner Awaiting requested review from gebner

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@lilyball @gebner @lheckemann