SpinalTemplateSbt icon indicating copy to clipboard operation
SpinalTemplateSbt copied to clipboard

Add structure for tests & fmt features

Open numero-744 opened this issue 2 years ago • 11 comments

Closes #28 and #26

:warning: Prepare an RTD update and merge the 2 PRs at the same time.

numero-744 avatar Dec 07 '22 19:12 numero-744

The problem is that source files in hw and tb directory would compile twice when execute a test & compile command. So that the dependency of this two targets have to be mixed, which is not a good practice for users.

Readon avatar Dec 08 '22 01:12 Readon

Why would it compile twice? I don't understand. IIUC it is exactly like main vs test of sbt structure, but in different paths.

numero-744 avatar Dec 08 '22 07:12 numero-744

Why would it compile twice? I don't understand. IIUC it is exactly like main vs test of sbt structure, but in different paths.

Emm, That might not be exactly.

I mean each time executing compile or test, it will compile all files found in the directory specified. So if files for design and test are put in the same directory, it will compile twice.

Readon avatar Dec 08 '22 09:12 Readon

Ah you mean the current situations where we put everything in hw/ directory and there is no tb/ directory?

numero-744 avatar Dec 08 '22 09:12 numero-744

re we put everything in hw/ directory and there is no tb/ directory?

Yes At the same time, I think the best way is not to modify scalaSource directly but create subproject in their corresponding locations by specify the relative directory in 'in file("xxxx/xxxxx")'.

Readon avatar Dec 08 '22 09:12 Readon

At the same time, I think the best way is not to modify scalaSource directly but create subproject in their corresponding locations by specify the relative directory in 'in file("xxxx/xxxxx")'.

I don't understand why. I don't know for mill but for sbt it seems that when we split main/test (or equivalently hw/tb) it splits into two compilation units (like two projects actually, projectname and projectname-test and re-compile only tests if only tests are modified)

numero-744 avatar Dec 08 '22 09:12 numero-744

At the same time, I think the best way is not to modify scalaSource directly but create subproject in their corresponding locations by specify the relative directory in 'in file("xxxx/xxxxx")'.

I don't understand why. I don't know for mill but for sbt it seems that when we split main/test (or equivalently hw/tb) it splits into two compilation units (like two projects actually, projectname and projectname-test and re-compile only tests if only tests are modified)

They are the same, but specify the sources directly might break the internal logic of the build tools. Like what I tried, if put some test code in one of the directories, it fails in many cases.

Readon avatar Dec 08 '22 09:12 Readon

I'm annoyed because I cannot reproduce… Can you provide an example, please?

numero-744 avatar Dec 08 '22 09:12 numero-744

At first, I would say I mixed some problems together, which is not good.

Now, here is some suggestion on this PR.

  1. I think the testbench, short as 'tb', should be a test, something with input and assertion. The existing one is much more an App than Test. Why there should be a unified runMain command, which is different from the lib's operation?
  2. I suppose that the best command we need is "sbt run" and "sbt test" for all design eleboration and test. Those simple command is easy to remember and try. Especially for people who are to start going.
  3. For ones that want to start their own project from this one, what they need to do is simply add their files in corresponding directory, and run above simple command.

Suggestions for the whole template:

  1. there are projectname in directory path to all scala files and in build.sbt files, it's really hard for them to find the locations need to be changed. If we just give them a default one like "all", so that the path could be "hw/spinal/MyTopLevel.scala" and "tb/spinal/MyTopLevelSim.scala". Of course, they can use a subdirectory to split their logics, that is what I have done in my own repositories.
  2. To be simple is also the reason why I think the internal variable alternation should be avoided. The internal variable like scalaSource is too implicit for reader to easily get and expand. Or we can add some comment to tell people, alter it is not often necessary.

My first SpinalHDL project is based on this and feels the directory structure is really good. I also understand that hardware people would like directories with "hw/rtl/tb" like structures. So let's keep it as simple as possible to make their modification easily.

Btw, I tried your PR with sbt "Test/runMain xxxx.xxxx" it works without multiple compilation. I will try to fix the Mill version.

Readon avatar Dec 08 '22 14:12 Readon

PR:

  1. I have just taken the existing one. I think it should be a test too, but IMO scalatest is too complex for new SpinalHDL users so having this simple App might be better IMO.
  2. I think yes because it compiles before prompting? This is more about documentation, if you open an issue in RTD I can add run in the getting started chapter, before runMain (I think instead of compile).
  3. :+1:

So I suggest, for now, to keep the simulation in hw as an App so that it can be simply run without Test/ prefix, and in the tutorial mention that tb/ is for tests; with something like "don't use it yet, you will learn how to use it later". And only once we have helpers to easily write scalatests we can introduce it earlier in the tutorial?

Template:

  1. I had discussed that with @Dolu1990 and he explained me that it is a Scala requirement to have the package name matching the folder, hence this directory. (I didn't add it in my first personal version of the template, however for me it worked).
  2. Have you read the "Change project structure" part of the readme? What do you think about it? Also I think quite of the opposite: to me, putting the variables in the configuration file makes them visible to the user so it should help understanding what should be modified with examples.

If you can fix mill build I'll thank you much! I'm really bad with mill, sorry I have just done something which seems to work, didn't do more testing :sweat_smile:

numero-744 avatar Dec 08 '22 20:12 numero-744

but IMO scalatest is too complex for new SpinalHDL users so having this simple App might be better IMO.

Agree That may be a layer too much.

Dolu1990 avatar Dec 08 '22 21:12 Dolu1990