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

Add gr-satellites #87

Closed
wants to merge 10 commits into from
Closed

Add gr-satellites #87

wants to merge 10 commits into from

Conversation

reiinakano
Copy link
Contributor

Currently, we only need one block from this but I imagine we'll need more in the future. Not sure if it's better to do this or to copy and paste what we need like before. Wdyt?

@anuraaga
Copy link
Contributor

It's better to use pybombs than copy-paste (if we can use pybombs for others we should switch to that).

The best pattern would be to finally add a recipe for starcoder in gr-recipes, gr-starcoder.lwr, which depends on gnuradio-nogui and gr-satellites and runs cmake (should happen automatically by extending the cmake recipe like most block recipes out there). Then we replace the command in installGrCStarcoder from a cmake invocation to pybombs install gr-starcoder. Do you want to try that out?

@reiinakano
Copy link
Contributor Author

Yeah that sounds better. I guess we would also need our own recipe of gr-satellites since it depends on gnuradio when we want to make it depend on gnuradio-nogui.

@anuraaga
Copy link
Contributor

I think we can set packages.gnuradio.forceinstalled: true in our recipe to prevent anyone we use from pulling in the monster.

@reiinakano
Copy link
Contributor Author

This'll probably fail because gr-starcoder is in a subdirectory of starcoder repo. I tried skimming through the code and examples and I didn't see a way to specify a subdirectory. Do you know a good way of solving this?

@anuraaga
Copy link
Contributor

Hmm - I don't think it should matter since we set that actual subdirectory as a recipe path
pybombs -y recipes add gr-starcoder ${project.file('gr-recipes')}

Does that cause issues?

@anuraaga
Copy link
Contributor

By the way, if you mean how would other users do it, I was thinking we'll probably need to make a separate gr-recipes repository once we're ready. First let's get it working in this build and then we'll move it.

@reiinakano
Copy link
Contributor Author

I don't mean adding the recipe, I mean in the recipe gr-starcoder.lwr itself, I'm setting source: git+https://github.com/infostellarinc/starcoder.

@anuraaga
Copy link
Contributor

Ah - I think you can set these to gr-starcoder/build

https://github.com/gnuradio/pybombs/blob/master/pybombs/templates/cmake.lwt#L20

This seems to work ok.

category: common
description: Out-of-tree Module for gr-starcoder
gitbranch: master
inherit: cmake
source: git+https://github.com/infostellarinc/starcoder.git
configuredir: gr-starcoder/build
makedir: gr-starcoder/build
installdir: gr-starcoder/build

@anuraaga
Copy link
Contributor

Also actually when looking though these files, I realized that we use the normal gnuradio recipe, just reconfigure it for our prefix. Users that just use normal gnuradio don't need to worry about it so the recipe should work both with our special nogui prefix or just a standard gnuradio install.

Reiichiro Nakano added 2 commits September 14, 2018 13:44
build.gradle Outdated
command """pybombs -vv config default_prefix gnuradio && \\
pybombs -vv -y recipes add-defaults && \\
pybombs -vv -y recipes add gr-starcoder ${project.file('gr-recipes')} && \\
pybombs -vv -y install gr-satellites
Copy link
Contributor

Choose a reason for hiding this comment

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

The prefix is already set up correctly so I don't think you need to call anything but install gr-satellites here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh - I mean gr-starcoder

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I actually get this warning without configuring the default prefix first.

PyBOMBS.ConfigManager.PrefixInfo - DEBUG - Cannot establish a prefix directory. This may cause issues down the line.

I still get an error with it but a different one right now..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I didn't npotice my typo, nvm


category: common
depends:
- gnuradio-nogui
Copy link
Contributor

Choose a reason for hiding this comment

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

gnuradio-nogui is actually a prefix, it doesn't work here and isn't really needed I guess.

- gnuradio-nogui
- gr-satellites
description: Recipe for installing the Starcoder OOT
config:
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this, the prefix is already set up correctly with gnuradio-nogui

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I remove the whole config? I still want to set pyconstruct to forceinstalled: true if possible

@reiinakano
Copy link
Contributor Author

reiinakano commented Sep 14, 2018

So it really doesn't seem like the prefix from setupPrefix is getting recognized in installGrStarcoder. If I try only pybombs install gr-starcoder it complains it can't find the gr-starcoder recipe. Then after adding the line pybombs -vv -y recipes add gr-starcoder ${project.file('gr-recipes')} && it complains it can't find gr-satellites. Then after adding pybombs -vv -y recipes add-defaults && it doesn't seem to honor the forceinstalled packages, which should have been setup by setupPrefix. Trying to set pybombs config default_prefix gnuradio doesn't work either.

@reiinakano reiinakano closed this Sep 14, 2018
@reiinakano reiinakano deleted the add-gr-satellites branch September 14, 2018 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants