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 structure for tests & fmt features #30

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

numero-744
Copy link
Collaborator

Closes #28 and #26

⚠️ Prepare an RTD update and merge the 2 PRs at the same time.

@Readon
Copy link

Readon commented Dec 8, 2022

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.

@numero-744
Copy link
Collaborator Author

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

@Readon
Copy link

Readon commented Dec 8, 2022

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.

@numero-744
Copy link
Collaborator Author

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

@Readon
Copy link

Readon commented Dec 8, 2022

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")'.

@numero-744
Copy link
Collaborator Author

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)

@Readon
Copy link

Readon commented Dec 8, 2022

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.

@numero-744
Copy link
Collaborator Author

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

@Readon
Copy link

Readon commented Dec 8, 2022

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.

@numero-744
Copy link
Collaborator Author

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. 👍

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 😅

@Dolu1990
Copy link
Member

Dolu1990 commented Dec 8, 2022

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.

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.

Add tests in the structure
3 participants