Skip to content

[svsim/ChiselSim] Discover the project's root directory in a sandboxed build #4931

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
May 21, 2025
Merged

[svsim/ChiselSim] Discover the project's root directory in a sandboxed build #4931

merged 3 commits into from
May 21, 2025

Conversation

kammoh
Copy link
Contributor

@kammoh kammoh commented May 20, 2025 •
edited
Loading

Newer versions of mill run tests inside a sandboxed folder under the 'out/' folder.

The current svsim/ChiselSim code does not consider this, resulting in a weird, deeply-nested, and hard to predict simulation path. This can be confusing for users. It's particularly annoying when trying to access trace files created by the simulation.

This is taken from #4911 with some modifications. #4911 will be updated after this PR is merged.

  • adds public getProjectRootOrCwd in svsim.Workspace as a more reliable way to discover the current project's root path, especially in the presence of build-system sandboxing.
  • adds workingDirectory to svsim.Simulation, resolved absolute path to workingDirectoryPath, resolved using getProjectRootOrCwd.
  • changes FileCheck, HasTestingDirectory, and WithTestingDirectory to use getProjectRootOrCwd

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Bugfix

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference).
  • Rebase: You will rebase the PR onto master and it will be merged with a merge commit.

Release Notes

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash) and clean up the commit message.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@kammoh
Try to discover the project's root directory in the presense of build…
d96c22b 
…-system sandboxing (e.g., as in the newer versions of mill).

- adds public `getProjectRootOrCwd` in `svsim.Workspace` as a more reliable way to discover the current project's root path
- adds workingDirectory to `svsim.Simulation`, resolved absolute path to `workingDirectoryPath`, resolved using `getProjectRootOrCwd`.
- changes `FileCheck`, `HasTestingDirectory`, and `WithTestingDirectory` to use `getProjectRootOrCwd`
seldridge
seldridge approved these changes May 20, 2025
View reviewed changes
Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

This seems like a much better way of handling this.

I've been annoyed/surprised by the deep placement of the build area in out/.

jackkoenig
jackkoenig reviewed May 20, 2025
View reviewed changes
svsim/src/main/scala/Workspace.scala Outdated
def getProjectRootOrCwd: Path =
sys.props
.get("chisel.project.root")
.orElse(sys.env.get("CHISEL_PROJECT_ROOT"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we consider enshrining CHISEL_PROJECT_ROOT elsewhere like here:

chisel/core/src/main/scala-2/chisel3/internal/SourceInfoFileResolver.scala

Line 18 in ff99b21

val projectRoot = sys.props.get("chisel.project.root")

Copy link
Contributor Author

@kammoh kammoh May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's good to have consistent behavior, but I'd do that in a separate PR.
For the record, the justification for such an environment variable is that the "native" versions of mill (and bloop), at least on macOS, seem to ignore -Dprop=value from both .mill-jvm-opts and the command line arguments. Therefore, it would be helpful to provide an alternative method for specifying this.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's annoying that native is ignoring -Dprop, will have to look into that at some point, would you mind filing an issue about that so when I forget I eventually notice the issue and remember again?

@kammoh
Copy link
Contributor Author

kammoh commented May 21, 2025

Thank you so much for the quick review!

@jackkoenig jackkoenig added the Bugfix Fixes a bug, will be included in release notes label May 21, 2025
@jackkoenig
Copy link
Contributor

@kammoh it does look like some tests are failing, some due to things that used to be relative paths not being relative anymore:

chisel[2.13.16].test.test 11 tests failed: 
  chiselTests.TraceSpec TraceFromAnnotations should be able to get nested name.
  chiselTests.TraceSpec TraceFromCollideBundle should work
  chiselTests.TraceSpec Inline should work should work
  chiselTests.TraceSpec Constant Propagation should be turned off by traceName
  chiselTests.TraceSpec Nested Module should work
  chiselTests.TraceSpec All traced signal should generate
  chiselTests.stage.WarningConfigurationSpec Warning Configuration File should support filters
  chiselTests.stage.WarningConfigurationSpec Warning Configuration File should support line comments
  chiselTests.stage.WarningConfigurationSpec Warning Configuration File should have good error messages
  chiselTests.stage.WarningConfigurationSpec Warning Configuration File should support leading whitespace
  chiselTests.stage.WarningConfigurationSpec Warning Configuration File should work when specified multiple times

jackkoenig
jackkoenig reviewed May 21, 2025
View reviewed changes
@kammoh
fix path in test
34d88b5 
No longer assuming `testingDirectory.getDirectory` is necessarily a
relative path
@kammoh kammoh force-pushed the sim_project_root branch from a2508b3 to 34d88b5 Compare May 21, 2025 03:39
@kammoh
Copy link
Contributor Author

kammoh commented May 21, 2025

@kammoh it does look like some tests are failing, some due to things that used to be relative paths not being relative anymore:

Oops! It should be fixed now.

@jackkoenig jackkoenig enabled auto-merge (squash) May 21, 2025 03:48
@jackkoenig jackkoenig merged commit 698c4a8 into chipsalliance:main May 21, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackkoenig jackkoenig jackkoenig left review comments

@seldridge seldridge seldridge approved these changes

Assignees
No one assigned
Labels
Bugfix Fixes a bug, will be included in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kammoh @jackkoenig @seldridge