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

.sbtopts file added + Check for BINARY/THIN_CLIENT flags #880

Merged
merged 5 commits into from
May 7, 2021

Conversation

abejgonzalez
Copy link
Contributor

Related issue: #833

Type of change: other enhancement

Impact: other

Release Notes
It seems like using the .sbtopts file helps with the thin SBT client (sets up the Chisel/Firrtl path properly). This PR changes Chipyard to be more similar to Rocket Chip and hopefully fixes #833 (or some subset of its problems). Additionally, this adds small check to make sure the BINARY flag is present when doing run-* targets.

@abejgonzalez
Copy link
Contributor Author

Tried to quickly fix the thin client issues with always giving 0 when using the sbt launcher (over the sbt script or sbtn). Unfortunately, I think the easiest way to solve this is for users to use the sbt script. So I'll punt this issue to later.

See description of how to start sbt for its different version: sbt/sbt#6468 (comment)

@abejgonzalez abejgonzalez changed the title .sbtopts file added + Check for BINARY flag .sbtopts file added + Check for BINARY/THIN_CLIENT flags May 6, 2021
@abejgonzalez
Copy link
Contributor Author

Any thoughts on this PR @tymcauley since you are the one who brought up .sbtopts issues in the past?

@abejgonzalez abejgonzalez requested a review from jerryz123 May 7, 2021 03:37
override SBT_OPTS += -Dsbt.sourcemode=true -Dsbt.workspace=$(base_dir)/tools
SBT_OPTS_FILE := $(base_dir)/.sbtopts
ifneq (,$(wildcard $(SBT_OPTS_FILE)))
override SBT_OPTS += $(subst $$PWD,$(base_dir),$(shell cat $(SBT_OPTS_FILE)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Is setting SBT_OPTS necessary now that .sbtopts exists? I was able to remove this code and run the start-sbt-server target just fine.

Copy link
Contributor

@tymcauley tymcauley May 7, 2021

Choose a reason for hiding this comment

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

Nevermind, this is necessary as I mentioned here: #880 (comment)

@tymcauley
Copy link
Contributor

Looks great! I also really like the check-* targets.

As I mentioned in that comment, on this branch I was able to run the start-sbt-server and shutdown-sbt-server targets without any errors. I was also able to run those targets when I removed the code in variables.mk which sets the SBT_OPTS command-line args for sbt. However, once I got rid of the .sbtopts file, the error from #833 happened again. So, maybe it's only necessary to have that file present?

@tymcauley
Copy link
Contributor

Ah, nevermind, I see sbt/sbt#6468 (comment) mentions that the sbt jar doesn't consume .sbtopts, but it will use the command-line args, so excluding the command-line args breaks the non-thin-client build.

Copy link
Contributor

@tymcauley tymcauley left a comment

Choose a reason for hiding this comment

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

LGTM!

@abejgonzalez abejgonzalez merged commit 92357f4 into dev May 7, 2021
@abejgonzalez
Copy link
Contributor Author

I do find it weird that launching SBT varies so much between the different versions. But in any case, I think this helps satisfy users who use the script vs the launch jar.

zslwyuan added a commit to zslwyuan/chipyard that referenced this pull request Aug 2, 2021
**Related issue**:  ucb-bar#932 and ucb-bar#880

**Type of change**: other enhancement

**Impact**: other (change a repo initialization script)

**Release Notes**
change a repo initialization script [init-submodules-no-riscv-tools-nolog.sh](https://github.com/zslwyuan/chipyard/blob/master/scripts/init-submodules-no-riscv-tools-nolog.sh) so it can generate the .sbtopt file in the Chipyard directory during repo initialization and use the absolute path for the SBT workspace variables in .sbtopts. 

Hope it can adapt to more compilation scenarios. For example, now users should be able to open the shipyard project directly using Intellij.
abejgonzalez referenced this pull request Aug 24, 2021
generate the .sbtopt file during repo initialization. ucb-bar#932 and ucb-bar#880
@abejgonzalez abejgonzalez deleted the misc-cleanup branch October 7, 2021 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants