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

soapysdr: fix modules-directory for extraPackages #53051

Merged
merged 1 commit into from
Jan 7, 2019

Conversation

betaboon
Copy link
Contributor

Motivation for this change
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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@timokau
Copy link
Member

timokau commented Dec 29, 2018

Is the wrapper actually necessary (i.e. did this issue break something)? If so, we should make sure it won't break in the future. If 0.7 comes from the version, we can generate it from the version with lib.version.minor and lib.version.patch.

Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

See comment.

@markuskowa
Copy link
Member

It looks like the wrapper is not needed anymore. I can run, for example, the remote server and welle-io without the wrappers.

@markuskowa
Copy link
Member

@betaboon can you please check if it works for you without the wrapper?

@betaboon
Copy link
Contributor Author

betaboon commented Jan 3, 2019

@markuskowa i will during the next couple of days ;)

@betaboon
Copy link
Contributor Author

betaboon commented Jan 5, 2019

@markuskowa I can't seem to get it to work without some kind of wrapper. how did you do it ?

@markuskowa
Copy link
Member

@betaboon You are right. It does not work without the wrapper. It found somehow the modules from my local installation. I would recommend to implement timokau's suggestion (see my source comment).

@betaboon
Copy link
Contributor Author

betaboon commented Jan 6, 2019

@markuskowa i made some more changes include your suggestion. how about it?

@markuskowa
Copy link
Member

@GrahamcOfBorg build soapysdr-with-plugins

${lndir}/bin/lndir -silent $i $out
mkdir -p $out/${modulesPath}
for package in ${toString extraPackages}; do
${lndir}/bin/lndir -silent $package/${modulesPath} $out/${modulesPath}
Copy link
Member

Choose a reason for hiding this comment

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

Now the binaries are not linked anymore. As a consequence binaries from the modules, such as SoapySDRServer, are not wrapped any longer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@markuskowa i was kindof annoyed by the collisions caused by symlinking all the packages into this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldnt it be better to wrap the binaries in the individual packages ?

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 just noticed, that we can use SOAPY_SDR_ROOT-environment-variable when we symlink everything into the soapysdr-package

Copy link
Member

Choose a reason for hiding this comment

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

There is always a little bit of a chicken-egg problem with the plugins, since they require the soapy library. With the modules we have at the moment, the whole wrapping is only needed for the SoapySDRServer from soapyremote. The binaries that come with limesuite are unnecessarily wrapped. All packages that will use soapy as backend and build against soapysdr-with-plugins do not need to worry, since SOAPY_SDR_ROOT is set at compile time and we have symlinked all modules into the base of soapysdr-with-plugins.
Suggestions: we take the version that you have now (symlinking only the lib modules) but also create a wrapper for the SoapySDRServer binary explicitly (in $out/bin).

The alternative could be to add an attribute wrappedBinaries to all plugins where it is excplicitly stated, which binaries should be symlinked and wrapped. However, since it is really only needed for one binary at moment, this seems to be a little bit of an overkill.

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 i find weird:

  • without wrapping SoapySDRServer running it wont find my limesdr.
  • when i export SOAPY_SDR_ROOT first it can find it, even without it being wrapped.
    which kind of contradicts SOAPY_SDR_ROOT being set at compile-time. but it obviously gets set at compile-time, either defaulting to CMAKE_INSTALL_PREFIX or by being set explicitly with cmakeFlags (which i tried, but doesn't make any difference).

so yeah... even tho i would prefer a cleaner(tm) solution we should stick to full symlinking as it as been before.
i will clean up this PR tomorrow.

@betaboon
Copy link
Contributor Author

betaboon commented Jan 7, 2019

@markuskowa i think I'm done here

@markuskowa
Copy link
Member

@GrahamcOfBorg build soapysdr-with-plugins

@markuskowa
Copy link
Member

Thanks for cleaning it up. Maybe a short clarification on why we have this mess with wrapping and module paths:

without wrapping SoapySDRServer running it wont find my limesdr.
when i export SOAPY_SDR_ROOT first it can find it, even without it being wrapped.
which kind of contradicts SOAPY_SDR_ROOT being set at compile-time. but it obviously gets set at compile-time, either defaulting to CMAKE_INSTALL_PREFIX or by being set explicitly with cmakeFlags (which i tried, but doesn't make any difference).

As I said above it is a chicken-egg problem. The final package is build in 3 steps:

  1. We build the naked soapysdr library. That one does not know anything about the modules/drivers
  2. We build all modules against soapysdr from step 1. The soapyremote module provides a driver but also the utility program SoapyRemote. To be useful it requires other modules but it the soapysdr package does not know about the other drivers. Setting SOAPY_SDR_PLUGIN_PATH, which points to the modules could make it work in the end.
  3. We recompile the library and symlink all the modules in the tree of soapysdr-with-plugins. This step is for convenience since SOAPY_SDR_ROOT now contains not only the library but also all driver modules. However, SoapyRemote is still linked against the old soapysdr which does not contain the modules, hence the wrapper with SOAPY_SDR_PLUGIN_PATH (setting SOAPY_SDR_ROOT at run time to ${soapysdr-with-plugins} should do the same job).

It is not the most beautiful solution but it gets the job done. I will try to come up with a cleaner solution in a future PR, since the wrapping is only relevant for the remote server.

@betaboon
Copy link
Contributor Author

betaboon commented Jan 7, 2019

@markuskowa thanks for clearing that up. now that i read your explanation in understood my misconception on one aspect.

Maybe in the future it would be a good idea to separate soapysdr-plugins into a module-package and a bin-package.

@markuskowa
Copy link
Member

Maybe in the future it would be a good idea to separate soapysdr-plugins into a module-package and a bin-package.

That is a good idea, I have not thought of that yet.

@markuskowa
Copy link
Member

@GrahamcOfBorg build soapysdr-with-plugins

@markuskowa
Copy link
Member

builds locally.

@markuskowa markuskowa merged commit cecec1f into NixOS:master Jan 7, 2019
@betaboon betaboon deleted the patch-2 branch January 8, 2019 10:07
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.

4 participants