search

olofk / fusesoc

Package manager and build abstraction tool for FPGA/ASIC development
BSD 2-Clause "Simplified" License
1.14k stars 238 forks source link

Libraries management. #290

Open m-kru opened 5 years ago

m-kru commented 5 years ago

I think that libraries management right now is not optimal and is quite fuzzy for user. I based my opinion on FuseSoc Documentation, personal experience and source code.

If I understand correctly fusesoc.conf file is responsible for storing locations of the libraries. If I am correct, here are my observations and suggestions:

  1. fusesoc init command is redundant and could be removed. fusesoc.conf file could be created when fusesoc is being installed or on any first execution of fusesoc library add or fusesoc update (about fusesoc update see also point 3).

  2. fusesoc init is dangerous/broken. If I add some libraries with fusesoc library add lib_name url, after fusesoc init, and then check fusesoc.conf everything is ok. The added libraries are in fusesoc.conf file. However, if I run fusesoc init again, by accident or on purpose, I end up with empty fusesoc.conf file. What is more, if I want to add library that already exists in .local/share/fusesoc, it fails (because there is directory for it in .local/share/fusesoc), and it does not create an entry in fusesoc.conf file.

  3. fusesoc update command is related to the libraries management, that is why I think it should be library command sub-command.

  4. User can add library with fusesoc library add my_lib url without specifying sync-type, but if he later executes fusesoc update there is an error ERROR: Invalid sync-type 'None' for library 'my_lib' - skipping.

  5. If user removes my_lib directory from .local/share/fusesoc, but leaves entry for that library in fusesoc.conf, and then runs fusesoc update , my_lib is not fetched (is not downloaded to the .local/share/fusesoc). I am not sure if such behavior is desirable, but I personally find it confusing. I think fusesoc update should fetch latest master for all libraries defined in fusesoc.conf file.

To sum up, my suggestion is to: remove fusesoc init command, create fusesoc.conf file during fusesoc installation, move update command as library command sub-command, fix bug/pitfalls of libraries management.

olofk commented 5 years ago

Thank you for this. I agree with most of what you say, and you're absolutely right that library management can be improved a lot. I'm right now prioritizing this bug since I stumbled across the fusesoc init issue that you mentioned where fusesoc.conf is left empty if you run it more than once. Nasty :(

1. `fusesoc init` command is redundant and could be removed. `fusesoc.conf` file could be created when fusesoc is being installed or on any first execution of `fusesoc library add` or `fusesoc update` (about `fusesoc update` see also point 3).

The goal with fusesoc init is to have an easy way to install the standard library while offering a way to operate without the standard library at all. Perhaps we should do it like this instead? If no fusesoc.conf is found (either in current dir or in ~/.config/fusesoc) then we ask if the user wants to create a fusesoc.conf with the standard library.

2. `fusesoc init` is dangerous/broken.  If I add some libraries with `fusesoc library add lib_name url`, after `fusesoc init`, and then check `fusesoc.conf` everything is ok.
   The added libraries are in `fusesoc.conf` file. However, if I run `fusesoc init` again, by accident or on purpose, I end up with empty `fusesoc.conf` file. What is more, if I want to add library that already exists in `.local/share/fusesoc`, it fails (because there is directory for it in `.local/share/fusesoc`), and it does not create an entry in `fusesoc.conf` file.

Yeah, this is not good at all. Just running fusesoc init twice will break stuff. As for a quick fix I think we should just abort fusesoc init (if it will still exist) when we detect an existing fusesoc.conf. As for the other issue of adding a lib that already exists in .local/share/fusesoc I have been planning to make some changes in this area. Right now you can add a --global flag to put the library in your global fusesoc.conf instead of the one in your workspace. I think that if you omit the --global flag, then the library itself should be downloaded to somewhere in the workspace instead. This will make it much easier to keep conf file and actual libraries in sync.

3. `fusesoc update` command is related to the libraries management, that is why I think it should be `library` command sub-command.

Yes, that's on my todo list.

4. User can add library with `fusesoc library add my_lib url` without specifying `sync-type`, but if he later executes `fusesoc update` there is an error `ERROR: Invalid sync-type 'None' for library 'my_lib' - skipping`.

Ouch. That's no good. Perhaps file a separate bug so that we don't lose track of this.

5. If user removes `my_lib` directory from `.local/share/fusesoc`, but leaves entry for that library in `fusesoc.conf`, and then runs `fusesoc update` , `my_lib` is not fetched (is not downloaded to the `.local/share/fusesoc`).
   I am not sure if such behavior is desirable, but I personally find it confusing. I think `fusesoc update` should fetch latest master for all libraries defined in `fusesoc.conf` file.

Ok, that's a bug. I'm pretty sure there was a discussion about this and the conclusion was to implement it just as you say. Again, perhaps file a specific bug report and keep this one for more high-level discussions

To sum up, my suggestion is to: remove fusesoc init command, create fusesoc.conf file during fusesoc installation, move update command as library command sub-command, fix bug/pitfalls of libraries management.

Sounds good. Let's get started :) Unfortunately I haven't had much time to work on FuseSoC lately so there's a bit of a backlog building up. Need to prioritize accordingly. The fusesoc init stuff is really bad and needs to be fixed asap. Then there's a demand from several places to start working on the use flags support

olofk commented 5 years ago

FYI, fixed 3 and made fusesoc init slightly safer

m-kru commented 5 years ago

Ok, I will create seperate issues for 4 and 5.

Coming back to 1. I personally do not find fusesoc standard library as standard. It is not like stdlib for C. I think it should be handled the same way as other libraries. If you want it, then add it with fuse library add ... after fusesoc installation. User would still have to execute one command after installation, but the overall number of commands for fusesoc would be reduced. In such case fusesoc.conf file could be created on the fusesoc library add ...call (if it does not exists of course). What do you think?

As you have respond to this issue I will take the opportunity to ask. Could you please check other issues and pull requests, both for FuseSoc and Edalize?

olofk commented 4 years ago

I'm beginning to agree with you. We need to tell the user to run fusesoc init so we could just tell them to run fusesoc library add --global fusesoc-cores https://github.com/fusesoc/fusesoc-cores instead if they want the base library.