-
Notifications
You must be signed in to change notification settings - Fork 651
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
Use Published Chisel, FIRRTL, Treadle, FIRRTLInterpreter packages instead of building from source. #1054
Conversation
b8a2986
to
9d97f8e
Compare
9d97f8e
to
804e3de
Compare
073ec7b
to
212503d
Compare
212503d
to
f5ac1a3
Compare
lazy val chipyardMandatedVersions = Map( | ||
"chisel-iotesters" -> "1.5.4", | ||
"firrtl-interpreter" -> "1.4.4", | ||
"treadle" -> "1.3.4", | ||
"chisel3" -> chiselVersion, | ||
"firrtl" -> firrtlVersion | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you using .4
for other things but 3.4.1
and 1.4.1
for chisel3 and firrtl? I'll note that using chisel-iotesters 1.5.4 might override the chisel3 to 3.4.4 anyway, you should check this in sbt with fullClasspath
to see what actual jars you're using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also possible that because you're giving chisel3 directly though it might pick your version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just picked the versions we had submoduled in. Looking at the class path FIRRTL 1.4.4 is showing up in some projects likely because of the first three dependencies. I can bump to chisel 3.4.4 and FIRRTL 1.4.4 but i was scared of changing too much, especially given this is going to be quickly followed by a 3.5 bump.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped to X.4.4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would confirm what Jack said. But overall LGTM
docs/Advanced-Concepts/Managing-Published-Scala-Dependencies.rst
Outdated
Show resolved
Hide resolved
docs/Advanced-Concepts/Managing-Published-Scala-Dependencies.rst
Outdated
Show resolved
Hide resolved
docs/Advanced-Concepts/Managing-Published-Scala-Dependencies.rst
Outdated
Show resolved
Hide resolved
docs/Advanced-Concepts/Managing-Published-Scala-Dependencies.rst
Outdated
Show resolved
Hide resolved
Co-authored-by: Abraham Gonzalez <abe.j.gonza@gmail.com>
d01e9cf
to
fa4a200
Compare
Co-authored-by: Abraham Gonzalez <abe.j.gonza@gmail.com>
a52d0bf
to
8666889
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There's still lots of cleanup that can be done, but this is most of the work.
Two warts:
TODO:
Related issue:
Type of change: other enhancement
Impact: build system
Release Notes