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

airwindows: init at unstable-2020-08-24 #89084

Closed
wants to merge 2 commits into from

Conversation

magnetophon
Copy link
Member

@magnetophon magnetophon commented May 28, 2020

Motivation for this change
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 added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels May 28, 2020
Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

See review comments for some improvements.

{ stdenv, fetchFromGitHub, requireFile, unzip, cmake }:

stdenv.mkDerivation rec {
name = "airwindows-${version}";
Copy link
Contributor

Choose a reason for hiding this comment

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

name is automatically constructed from pname-version.

Suggested change
name = "airwindows-${version}";
pname = "airwindows";

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

Choose a reason for hiding this comment

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

@OPNA2608 I seem to have messed up the push, sorry.
I'm working on getting the new fork to build, and once will than push the fixes.

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 had some trouble pushing to this branch, so I made #123954

pkgs/applications/audio/airwindows/default.nix Outdated Show resolved Hide resolved
sha256 = "1ya4qbc63sb52nzisdapsydrnnpqnjsl5kgxibbr3dxf32474g89";
};
installPhase = "cp -r . $out";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

You can add the unfree license here and keep the main derivation MIT.

Suggested change
};
meta.license = stdenv.lib.licenses.unfree;
};

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback, I'm working on it.
I think you mean line 23, in vst-sdk, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Err yea, apologies.

sha256 = "1mh7c176fdl5y5ixkgzj2j2b7qifp3lgnxmbn6b7bvg3sv5wsxg3";
};

vst-sdk = stdenv.mkDerivation rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

I see this duplicated over in the already existing airwave distribution, just in a different version.

vst-sdk = stdenv.mkDerivation rec {
name = "vstsdk368_08_11_2017_build_121";
src = requireFile {
name = "${name}.zip";
url = "http://www.steinberg.net/en/company/developers.html";
sha256 = "e0f235d8826d70f1ae0ae5929cd198acae1ecff74612fde5c60cbfb45c2f4a70";
};
nativeBuildInputs = [ unzip ];
installPhase = "cp -r . $out";
meta.license = stdenv.lib.licenses.unfree;
};

And a PR with the SDK part updated to the very version you're requiring here, #82380

I think I'd rather see this as a separate derivation, but there were objections to that a long while ago (#10006). I wonder if these still exist? Just commenting on this situation, not requesting any change in particular.

@magnetophon magnetophon changed the title airwindows: init at 2020-05-25 airwindows: init at unstable-2020-05-25 May 28, 2020
@magnetophon
Copy link
Member Author

I just found https://github.com/laserbat/airwindows, created a few hours ago.
I wonder if I should switch.
What do the creators think? @laserbat @ech2

@laserbat
Copy link

laserbat commented Jun 3, 2020

Hi @magnetophon

I don't think it would b a good idea to switch, as my fork is for personal use and might not be 100% compatible with originals.

I can, however, make a branch that contains no changes other than improved build system for linux. Would that be helpful?

@magnetophon
Copy link
Member Author

@laserbat Thanks for the feedback and the offer.
I don't know if it would be helpful.
I guess it depends on how up to date it is kept and how much it improves the build system.

@laserbat
Copy link

laserbat commented Jun 5, 2020

Well, current build system for linux doesn't really work without some patching and even with patches provided by some people in pull requests it has certain issues (mainly very slow build time). I rewrote the cmake setup so that's it's quick and painless to build and doesn't need any changes besides downloading VST SDK files.

Anyway, here's the branch in question https://github.com/laserbat/airwindows/tree/new_build

I do plan to keep it up to date. I'll set up a cron script that merges new changes from upstream into this branch, since upstream author only changes source files and not build configuration.

Hope this helps.

@magnetophon
Copy link
Member Author

That sounds really good, thank you!
I'm finishing up some other stuff and then I'll base this package on your branch.

@magnetophon
Copy link
Member Author

@laserbat
When I try to build that tree, I get:

-- Configuring done
CMake Error at CMakeLists.txt:19 (add_library):
  Cannot find source file:

    /tmp/nix-build-airwindows-2020-06-05.drv-0/source/plugins/LinuxVST/vstsdk/vstplugmain.cpp

  Tried extensions .c .C .c++ .cc .cpp .cxx .cu .m .M .mm .h .hh .h++ .hm
  .hpp .hxx .in .txx


CMake Error at CMakeLists.txt:26 (add_library):
  No SOURCES given to target: uLawEncode


CMake Error at CMakeLists.txt:26 (add_library):
  No SOURCES given to target: uLawDecode


CMake Error at CMakeLists.txt:26 (add_library):
  No SOURCES given to target: DeRez


CMake Error at CMakeLists.txt:26 (add_library):
  No SOURCES given to target: Golem

And then a whole lot more similar lines.
Any idea?

@magnetophon
Copy link
Member Author

@laserbat I just noticed airwindows/airwindows@3c0d151
So it seems he does take external input, at least from you! ;)
Have you considered doing a PR with your build changes? (assuming the above error is a NixOS issue)

@laserbat
Copy link

laserbat commented Jun 9, 2020

@magnetophon unfortunately he is too busy to work on the build system right now. I offered him help, but he didn't seem to have the time.

And the build fails because you need to download VST SDK first, there's a script named get_vst_sdk.sh included in the LinuxVST folder.

Unfortunately Steinberg are really vigilant in DMCAing anybody who has this code in their repository, so it has to be downloaded every time before building.

@magnetophon
Copy link
Member Author

@laserbat Thanks. I still can't get it to build though, sorry.

When I run the commands in that script and then build, I get:

CMake Error at CMakeLists.txt:12 (add_subdirectory):
  add_subdirectory given source "include/vstsdk" which is not an existing
  directory.


CMake Error at CMakeLists.txt:13 (add_airwindows_plugin):
  Unknown CMake command "add_airwindows_plugin".


-- Configuring incomplete, errors occurred!

When I change the commands to use "include/vstsdk" instead, I get:

CMake Error at CMakeLists.txt:12 (add_subdirectory):
  The source directory

    /tmp/nix-build-airwindows-unstable-2020-06-05.drv-0/source/plugins/LinuxVST/include/vstsdk

  does not contain a CMakeLists.txt file.


CMake Error at CMakeLists.txt:13 (add_airwindows_plugin):
  Unknown CMake command "add_airwindows_plugin".


-- Configuring incomplete, errors occurred!

@laserbat
Copy link

My bad! I fixed this issue

@magnetophon
Copy link
Member Author

Ah, yes, now it builds. Thank you, I thought I was going crazy!
Any chance you will write an install target in the makefile?

@magnetophon
Copy link
Member Author

@laserbat I installed manually, but Ardour has now blacklisted all of them.
How do I find out why?

@magnetophon magnetophon changed the title airwindows: init at unstable-2020-05-25 airwindows: init at unstable-2020-08-24 Aug 27, 2020
done;
'';

meta = with stdenv.lib; {
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
meta = with stdenv.lib; {
meta = with lib; {


nativeBuildInputs = [ cmake ];

patchPhase = ''
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
patchPhase = ''
postPatch = ''

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 had some trouble pushing to this branch, so I made #123954

cp -r ${airwindows-ports}/include/vstsdk/CMakeLists.txt include/vstsdk/
cp -r ${vst-sdk}/pluginterfaces include/vstsdk/pluginterfaces
cp -r ${vst-sdk}/public.sdk/source/vst2.x/* include/vstsdk/
chmod -R 777 include/vstsdk/pluginterfaces
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
chmod -R 777 include/vstsdk/pluginterfaces
chmod -R 755 include/vstsdk/pluginterfaces

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants