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.

Sign up for GitHub

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

mission-planner: init at version 1.3.74 #107545

Merged
merged 1 commit into from Dec 27, 2020
Merged

mission-planner: init at version 1.3.74 #107545

merged 1 commit into from Dec 27, 2020

Conversation

wucke13
Copy link
Contributor

@wucke13 wucke13 commented Dec 24, 2020

Motivation for this change

Configuring my Ardupilot FC

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.

@wucke13
Copy link
Contributor Author

wucke13 commented Dec 24, 2020

@GrahamcOfBorg build mission-planner

@ofborg ofborg bot requested a review from edolstra December 24, 2020 18:03
SuperSandro2000
SuperSandro2000 reviewed Dec 24, 2020
View reviewed changes
pkgs/applications/science/robotics/mission-planner/default.nix
Comment on lines +28 to +30
for file in $AOT_FILES
do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for file in $AOT_FILES
do
for file in $AOT_FILES; do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the rational here? What makes one better than the other in you point of view?

Copy link
Member

Choose a reason for hiding this comment

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

It uses one line less and is just a common thing I do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I prefer GNU C style, and similar to that, I think having the loop bodies opening symbol in the next line makes the code more readable.

pkgs/applications/science/robotics/mission-planner/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/robotics/mission-planner/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/robotics/mission-planner/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/robotics/mission-planner/default.nix Outdated Show resolved Hide resolved
pkgs/applications/science/robotics/mission-planner/default.nix

installPhase = ''
# create target directoryies
mkdir -p $out/{bin,opt/mission-planner}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
mkdir -p $out/{bin,opt/mission-planner}
mkdir -p $out/bin $out/opt/mission-planner

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why is more repetition better?

Copy link
Member

Choose a reason for hiding this comment

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

Using {..,..} is nice if only a small part of a path changes like the last directory in a 5 deep nested path. I don't think it makes the code more readable if $out/ is the only repeating part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disagree on this one as well, but it likely boils down to personal preference. Is it a pressing issue to you having this changed?

pkgs/applications/science/robotics/mission-planner/default.nix Outdated Show resolved Hide resolved
@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 107545 run on x86_64-darwin 1

1 package built:
  • mission-planner

@doronbehar
Copy link
Contributor

There's an editorconfig issue also.

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 107545 run on x86_64-linux 1

1 package built:
  • mission-planner

@wucke13
Copy link
Contributor Author

wucke13 commented Dec 25, 2020

@SuperSandro2000 @doronbehar Thank you two for the feedback so far, I adjusted some of the issues you mentioned. Hope it works out now!

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 107545 run on x86_64-linux 1

1 package built:
  • mission-planner

@doronbehar doronbehar merged commit 6f8710a into NixOS:master Dec 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@doronbehar doronbehar doronbehar requested changes

@edolstra edolstra Awaiting requested review from edolstra

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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@wucke13 @SuperSandro2000 @doronbehar