olofk / fusesoc

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

Provide option to link source files, instead of copying them #165

Closed imphil closed 6 years ago

imphil commented 7 years ago

Currently fusesoc copies all input source files to build/XXX/src and runs the build from there. That's a suitable behavior for fully automated builds. However, during the development phase, I'm have a process like that:

  1. Create some HDL code
  2. Run fusesoc build --setup
  3. Run vivado with GUI to add ILA probes, and then generate bitstream
  4. rinse and repeat, i.e. fix bug in code and re-run synthesis

That process is currently rather annoying, as changes I make in my source tree are not reflected in the code Vivado sees, and the other way around.

So I'm proposing to add an option to fusesoc to symlink the source files instead of copying them. That solves the problem nicely.

It's, however, not a solution without edge cases:

I've pushed the code I'm using right now to #164 (all the diff is refactoring of this function, just the last line is the actual change). It requires more discussion first before I can make it a real PR:

olofk commented 7 years ago

I know that the use-case you describe is a pain with FuseSoC today. I have been encountering it myself, but you know what... I have another approach for this problem. I was actually thinking about adding it as a final commit for v1.7, but decided against it because I want more testing.

My approach is to optionally just skip the export stage and reference the files from the original locations. This is also a behaviour that I think many people are actually expecting, so it would be a nice service to them as well. All the code is already in place, I just need to add a command-line switch (been planning to go with --no-export) and hook it up in main. The test suite already works in unexported mode for most of the test cases. If you want to try it yourself you can just change this to export=False and hopefully it should work.

The things that might break from this are (AFAIK)

Other than that, I think it should be good.

What do you think about that?

imphil commented 7 years ago

That should work as well. I'll give it a try next week and see how it works in practice. Benefit of your solution (using absolute paths) is that is also works for Windows.

Without looking at the code, I think fusesoc doesn't always ensure that absolute paths are used in the generated build TCL files. I'll give it a try first, however, before I complain about that :)

imphil commented 7 years ago

Yeah, as expected, setting export=False still puts relative paths into the generated Vivado tcl file, which doesn't work. So we'll need to convert those into absolute paths when using --no-export. When the files are exported, using relative paths (as it is now) is beneficial since it allows projects to be relocated (moved to a different location) without the project file breaking.

wallento commented 7 years ago

Vivado modifies constraint files to add ILA probes (mark_debug commands). That means also constraint file in the source tree gets modified with such debug-only changes. Again, that's acceptable to me for two reasons: first, it's the same behavior users of Vivado without fusesoc get, and second, it's easy to see and revert such changes in the source tree if using git or any other SCM.

@imphil Actually you can force Vivado to not do that:

  1. create a new xdc file in the development tree
  2. go to sources and right click on the xdc, then Set as Target Constraint File
  3. voila, only this one is used now for the debug probes
olofk commented 7 years ago

Ah yes. That's how I did it as well last time I used Vivado. Incredibly stupid of Vivado to overwrite people's constraints files by default. Especially as it reformats the rest of the file as well.

I had no idea that there was also a problem with using relative paths instead of absolute ones. I would prefer to use relative even in the --no-export case. The biggest reason for that is that it still makes the test suite independent of the environment and that some tool (I think Quartus qsys) actually doesn't work with absolute paths IIRC

wallento commented 7 years ago

Is that because it expects them to start with C:\? Okay, I shouldn't joke..

imphil commented 7 years ago

We can use relative paths, but then they still need to point to the original source files, not the exported (but but actually non-existant) files. Right now, the paths are like ../src/cleaned-module-version-0/verilog/something.v where they should be either /home/philipp/project/verilog/something.v or a relative version of that.

olofk commented 7 years ago

Is the current --no-export sufficient for that (with the target constraints file workaround that @wallento mentioned)? I want to add the frontend option, so this is an excellent case for me to know if there are any unknown problems

wallento commented 7 years ago

From my limited point of view they are similar to the user. The --no-export option has the advantage that it doesn't rule out Windows per se, right?

By the way, just thinking a bit beyond that, how about adding a metadata file into the exported directory with a map containing the file origins. Not only the part, but also module and version information. I think this may be useful. But that is something different, @olofk if you think this may make sense I can create a "good first bug" issue for that.

imphil commented 7 years ago

So yes, introducing a no-export option would work for me. If the final tcl file contains absolute paths or relative paths to the same (original source) file doesn't really matter to me.

Either way, the implementation needs more than setting just the export=False flag, but we also need to touch the logic to generate the correct file paths as used in the tcl file.

olofk commented 7 years ago

@wallento Good idea. I'm planning to dump the EDA API struct to disk and let backends read that instead of sending it as an argument to the backends. I could extend that to contain more metadata. Not sure it's a good first bug as I would like to do some heavy thinking about what to include in EDA API first

@imphil Good to know that it would help in your use-case. Not sure I understand the last part however. Are you specifically talking about the Vivado-generated debug TCL files, or TCL files in general?

bqwer commented 7 years ago

I second this request. After transferring two projects (about 20 cores) to fusesoc our team have found same difficulties when using Vivado to debug and correct sources. Vivado helps save time with simulation and debug but it's pain to remember what files from which cores you changed in process and to manually copy them back. Option like --no-export with support for projects with dependencies will help alot.

olofk commented 7 years ago

Your wish is my command. I have just pushed d9c4808d551423fbe562265d3b9dcc3b56e42459 which adds the --no-export option. Please close this bug if it solves your issues, or let me know what to fix

bqwer commented 6 years ago

We've tested it and it seems to work so far. I'm not big fan myself, but other members of our team are quite happy with it. It has some drawbacks. For example, vivado produces output products in xci ip cores directories, but git clean -fd helps a lot if sources are under git control.

One thing that is missing is ability to build project template without commands to run implementation. It helps to support project flow as runs wouldn't start when project is opened. But it's not too difficult to implement. I think we can do it later. Something like --setup flag to produce project tcl without build commands and --setup-build with them.

olofk commented 6 years ago

Yeah, you're right. It does make the build a bit more messy when things are executed directly from the original source. Exporting to a build directory was supposed to avoid that in the first place. One potential solution could perhaps be to let the vivado backend always copy all xci files somewhere below the work_root before executing, even if --no-export is used.

About the second part, that's an interesting idea. I find myself starting a build and aborting whenever I want to produce the project files for the quartus backend. I think the easiest way for now would be to make the vivado backend produce a Makefile where one target could be to just build the project files. Ideally, I want all backends to produce makefiles anyway, so that it's easy for users to run builds in a build tree externally from FuseSoC

olofk commented 6 years ago

@bqwer There is now a copyto attribute available for files to copy them do a destination relative to work_root when setting up the system. You can use this for the xci files to avoid polluting your source repos. You do it like this in the .core file files = my_ip.xci[file_type=xci,copyto=my_ip/my_ip.xci] This will copy the file to $work_root/my_ip/my_ip.xci It's recommended to keep each xci file in a separate sub dir as Vivado seems to get angry when there are multiple xci files in a single dir

Feel free to open a separate bug to add support for generating IP files without launching a build

Can we close this issue now or are there other outstanding problems?

imphil commented 6 years ago

Revisiting this in the hope to finally close it. Why revisit it? The current --no-export option doesn't work well for XCI files in Vivado for a couple of reasons: first, Vivado modifies the XCI files during the build/upgrade_ips process (that would be tolerable in the world of git), second it tries to write additional files (*.xml) next to the xci files during the build process, and third the build fails if there are multiple XCI files in the same directory (IPs are reported as "locked").

I'm looking for a solution which allows me to build with and without copying the source files (i.e. release/development mode), and making sure that we have clear rules how to structure *.core files in a way to support that. Ideally, these rules would be the same as they are today, but things like "only one XCI file per directory" is fine if it's clearly documented or even enforced by fusesoc.

So I'm looking at what options we have:

  1. copyto. If I understood your comment correctly @olofk I'd add copyto to all XCI files and this would copy them to the build directory even if I specify --no-export. Is that understanding correct? I don't really love this option as it would require core authors to agree on a non-conflicting directory scheme in the copyto argument to not end up in the situation where two IPs are copied to the same directory. (Since I don't necessarily control all *.core files I make use of this is a bit fragile.)
  2. Modify --no-export to "not copy" only certain file types, but copy others. I'm thinking about something like --no-export=hdl (a newly introduced keyword hdl to match all HDL source files), --no-export=.v,.sv,.vhdl (a match on file extensions), or --no-export='*verilogSource*' (a match on the file_type property).
olofk commented 6 years ago

Regarding 1, yes that is correct. copyto will copy regardless. This is because some files must be assumed to exist in the working directory. I agree with the criticism that copyto doesn't protect from several cores writing to the same location. I thought a lot about that and different schemes to get around it, but they all become increasingly complicated with little benefit. In practice I find it unlikely that two cores will use copyto with the same destination file. If that happens, it will likely be detected and if both core authors refuse to change their name, you could overload one of the cores with a modified .core file that uses another name in worst case. To mitigate this further I should probably add some best practices documentation that tells users to voluntarily namespace their files with a sanitized VLNV or similar.

It should be technically possible to do 2. I haven't thought this through, but it does feel a bit strange to use the file type as a filter (file type is definitely better than matching suffices or trying to capture files under a hdl alias.). My preferred solution here is still to use the copyto attribute, but we can still elaborate on the second idea to see if file type is the best filter. What you really want to do in this particular case is more like only export the xci files, and that's basically what copyto does

imphil commented 6 years ago

I went with the rather simple approach to make sure all XCI files are in their own directory. Together with appropriate .gitignore files this works well in my initial testing in OpTiMSoC. (Commit here: https://github.com/optimsoc/optimsoc/commit/5e047357f4abab021302b9da7ab03ab2afdaa7d3). Following the convention of "one XCI file per directory" should be sufficient, ideally this would be documented or even warned about or enforced by fusesoc. Closing this long-standing issue now for good.