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

virtual-ans: init at 3.0.2c #90624

Merged
merged 1 commit into from Aug 22, 2020
Merged

virtual-ans: init at 3.0.2c #90624

merged 1 commit into from Aug 22, 2020

Conversation

jacg
Copy link
Contributor

@jacg jacg commented Jun 16, 2020

Motivation for this change

No Nix package for virtual-ans exists yet.

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.

@jacg
Copy link
Contributor Author

jacg commented Jun 16, 2020

The checks appear to fail because they are running on arm. This package does not support arm. I have no idea how to proceed.

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Few suggestions :)

pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
@jacg
Copy link
Contributor Author

jacg commented Jun 16, 2020

Many thanks for your help @IvarWithoutBones!

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Few more minor nits, otherwise LGTM.

Also, commits should be squashed together into one, with a message following the contributing guidelines.

pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
pkgs/applications/audio/virtual-ans/default.nix Outdated Show resolved Hide resolved
Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

Looks good to me :)

@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-in-distress/3604/29

@Lassulus
Copy link
Member

there seem to be a lot of unneeded files in the top-level of the generated output. are they really necessary? for example: START_LINUX_X86 or boot.pixicode

@jacg
Copy link
Contributor Author

jacg commented Aug 22, 2020

boot.pixicode is definitely needed: it's the code that gets executed on the virtual machine on which the whole thing is built.

I've hacked together a scheme for removing all START_* scripts which don't correspond to the current architecture. There's probably a better way to do it, but it seems to work.

@Lassulus
Copy link
Member

ah, ic, I thought it was just left over files. You don't need to remove the unneeded files then, since the code seems more complex than having the few files lying around.

@jacg
Copy link
Contributor Author

jacg commented Aug 22, 2020

I've shortened and improved the removal logic, so that it should automatically work if darwin is ever enabled.

As it stands it will needlessly keep START_LINUX_X86 if the architecture is 86_64, but at 440 bytes, I'm not too bothered, compared to START_WINDOWS/MACOS.app which weigh around 2M each.

It would be more important to get rid of bin/pixilang_linux_x86{,_64} when unused, as these also occupy around 2M each. (It looks like the useful total for any single architecture is about 12M, so they increase the size of the package by a useless 17% on Linux and 33% on Darwin.)

@jacg jacg force-pushed the virtual-ans branch 2 times, most recently from 82735bf to 84bbb10 Compare August 22, 2020 16:43
@jacg
Copy link
Contributor Author

jacg commented Aug 22, 2020

With the last change, I think we're now keeping close to the minimum required for any single architecture.

@Lassulus
Copy link
Member

cool, can you squash the changes into one single commit? no need to keep the history I guess

@jacg
Copy link
Contributor Author

jacg commented Aug 22, 2020

can you squash the changes into one single commit?

Done.

@Lassulus Lassulus merged commit e56d1e2 into NixOS:master Aug 22, 2020
@jacg jacg deleted the virtual-ans branch August 22, 2020 19:29
@jacg
Copy link
Contributor Author

jacg commented Aug 22, 2020

Thanks @Lassulus for your herculean effort on merging the PR backlog!

@Lassulus
Copy link
Member

Thanks @Lassulus for your herculean effort on merging the PR backlog!

its more of an sisyphean effort :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants