-
Notifications
You must be signed in to change notification settings - Fork 5
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
Keep only the root and core project #1021
Conversation
Easy to do since ribasim_cli is a thin wrapper over Ribasim.main nowadays.
This way we always have them ready for interactive use.
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.
If the tests work, it can be merged from my side.
Nice yak shave
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 have some questions about the root Project.toml
MarkdownTables = "1862ce21-31c7-451e-824c-f20fa3f90fa2" | ||
MetaGraphsNext = "fa8bd995-216d-47f1-8a91-f3b68fbeb377" | ||
OrderedCollections = "bac558e1-5e72-5ebc-8fee-abe8a469f55d" | ||
OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed" | ||
PackageCompiler = "9b87118b-4619-50d2-8e1e-99f35a4d4d9d" | ||
PreallocationTools = "d236fae5-4411-538c-8e31-a6e3d9e00b46" | ||
ReTestItems = "817f1d60-ba6b-4fd5-9520-3cf149f6a823" | ||
Revise = "295af30f-e4ad-537b-8983-00126c2a3abe" | ||
Ribasim = "aac5e3d9-0b8f-4d4f-8241-b1a7a9632635" |
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 see that Ribasim itself is still in this list. Does that make sense?
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.
Yes. The root project is the superset of the core + all other dev/build/other deps we need. Including Ribasim here means we can use Ribasim from the root environment.
MarkdownTables = "1862ce21-31c7-451e-824c-f20fa3f90fa2" | ||
MetaGraphsNext = "fa8bd995-216d-47f1-8a91-f3b68fbeb377" | ||
OrderedCollections = "bac558e1-5e72-5ebc-8fee-abe8a469f55d" | ||
OrdinaryDiffEq = "1dea7af3-3e70-54e6-95c3-0bf5283fa5ed" | ||
PackageCompiler = "9b87118b-4619-50d2-8e1e-99f35a4d4d9d" | ||
PreallocationTools = "d236fae5-4411-538c-8e31-a6e3d9e00b46" | ||
ReTestItems = "817f1d60-ba6b-4fd5-9520-3cf149f6a823" | ||
Revise = "295af30f-e4ad-537b-8983-00126c2a3abe" |
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.
The split made it possible to add debug items that would not be needed to package in the binary. How do we know if we don't pack too many dependencies now? Should we remove some developer only packages and set it up in a different way?
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.
The root project is used to build the binaries, but it is only the core project that is being built. So there is no risk to adding more dev dependencies to the root project.
Before this change we did also have libribasim
and ribasim_cli
projects. So we do lose the ability to add dependencies for our binaries that are not depdendencies of the core. But there are no such dependencies right now.
In #1021 I enabled to run CompatHelper on the root project, but seeing the flood of PRs this morning and thinking about it some more, I think this was a mistake. We should close all those PRs. Compat is needed in core for all dependencies. For other dependencies, in principle we shouldn't declare compat, since we use a Manifest anyway. This is similar to the way we allow any version in pixi.toml, and the monthly pixi.lock update also brings in possibly breaking changes that we can then evaluate. We could have compat entries in the root project. Those makes sense if we are not compatible with newer breaking releases to hold it back. This way the manifest update will not bring in this incompatible release. We should not forget about these, so ideally we have an issue labeled `tech-debt` for those.
Taken out of #1204 whilst that is on hold. The directories changed in #1021, causing our precompile run to return error code 1 with a "TOML not found error". That means we were not precompiling an actual run and therefore probably need to wait a bit longer before the start of a simulation. This fixes the directory and adds an assertion to make sure the simulation is successful.
This attempts to simplifiy our Julia project and build setup.
Where possible, existing projects are moved into the root project like in #1014. All core dependencies are now also direct root dependencies to make using the root project more convenient for development.
While working on this I realized that we can remove the
libribasim
andribasim_cli
projects entirely.libribasim
is now a module of the Ribasim package, andribasim_cli
already was just a tiny wrapper aroundRibasim.main
. The content of the README.md files from those two folders now reside in thebuild_lib
andbuild_app
docstring.Since that left only the
create_binaries
folder under/build
, I collapsed that level out.create_binaries
is no longer a package, but the code is still used, but now simply directly included inbuild.jl
.