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

Feature Request: New component-test-depends section #7505

Closed
newhoggy opened this issue Aug 3, 2021 · 18 comments
Closed

Feature Request: New component-test-depends section #7505

newhoggy opened this issue Aug 3, 2021 · 18 comments

Comments

@newhoggy
Copy link

newhoggy commented Aug 3, 2021

Describe the feature
Currently we have the build-tool-depends feature. This is normally used to ensure build tools are built so that the build tool can be used during the build process.

We have been abusing this feature for another purpose which is to ensure that executables are built before tests are run. This is useful for CLI tests or integration tests where you would want the CLI executable to integration component to be already built before running the test.

This is especially useful to ensure that we aren't accidentally picking up stale executables that haven't been built which is a problem we've experienced in the development process and caused much confusion.

Unfortunately, build-tool-depends does a lot more than just ensuring the executable is built, which incurs additional costs. Specifically, it seems to significantly at to constraint resolution times. For example:

With build-tool-depends:

cabal build --dry-run all  15.44s user 0.61s system 95% cpu 16.828 total

Without build-tool-depends:

cabal build --dry-run all  0.48s user 0.10s system 83% cpu 0.699 total

On Windows, for some reason we are forced to use reorder-goals or the constraint resolution fails, but this can bring the constraint resolution time to about 20 minutes.

If we could avoid using build-tool-depends, we hope to cut constraint resolution times by 66%, which is a substantial gain.

@angerman
Copy link
Collaborator

angerman commented Aug 3, 2021

@Mikolaj can we prioritise this please?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 3, 2021

@newhoggy: aren't the times you are showing swapped? Is the runtime with build-tool-depends really shorter? [edit: fixed now]

Is the goal to have a cheaper variant of build-tool-depends that only ensures executable is freshly built or are there any extra requirements? Are your savings supposed to emerge from component-test-depends being cheaper or from not using either section in some cases?

@michaelpj
Copy link
Collaborator

Wouldn't this be better solved by making build-tool-depends not be slow, rather than by adding a new feature that does the same thing?

@Mikolaj
Copy link
Member

Mikolaj commented Aug 3, 2021

@newhoggy: while we are at that, any easy way to reproduce the problem? Strangely I can't find any mention of build-tool-depends in the repo with the reproducer for #7472? @michaelpj: what am I missing?

@jneira
Copy link
Member

jneira commented Aug 3, 2021

Related issues asking for similar functionality:

There is a a pattern here, maybe an opportunity to make something more generic?
It seems this is quite similar to run-tool-depends

@fgaz
Copy link
Member

fgaz commented Aug 3, 2021

This does looks like a run-tool-depends with no separate build plan.

I wonder though if your original problem could be solved in a more straightforward way, namely moving the executable code in an internal library, using that in build-depends in the text stanza, and calling the main function in the test code with the appropriate arguments/environment/argv. Would this be ok and if not why? Are there multiple tools that call each other?

@newhoggy
Copy link
Author

newhoggy commented Aug 3, 2021

This does looks like a run-tool-depends with no separate build plan.

Yes. That sounds right.

I wonder though if your original problem could be solved in a more straightforward way, namely moving the executable code in an internal library, using that in build-depends in the text stanza, and calling the main function in the test code with the appropriate arguments/environment/argv.

Would this be ok and if not why? Are there multiple tools that call each other?

They're not really tools, but various executables that communicate with each other via the various kinds of IPC and form a larger system.

Each executable component may have an interface includes things like command line arguments, environment variables, various kinds of IPC (filedescriptors/namedpipes/socket/signals) etc, some of which are OS specific.

The idea is to integration test them using Haskell code.

It is better that these components remain as separate executables because that's how they'll be used in the real world and killing a process provides a good way to cleanup OS resources in a manner that killing a thread does not provide.

Being able to clean up OS resources reliably provides an important means of test isolation.

We also have regression tests which invoke a shell script from Haskell tests which in turn invoke various executables with the goal of ensuring the shell scripts aren't broken by commits.

@michaelpj
Copy link
Collaborator

@newhoggy: while we are at that, any easy way to reproduce the problem? Strangely I can't find any mention of build-tool-depends in the repo with the reproducer for #7472? @michaelpj: what am I missing?

Right here: https://github.com/michaelpj/cabal-build-tool-depends-solver-repro/blob/51fc88e78d347eab41be6fd4a3b3500757dc942c/C/C.cabal#L11

This does looks like a run-tool-depends with no separate build plan.

run-tool-depends would be nice, in that it would allow us to be more precise about the fact that we need this at runtime rather than build time (this has consequences for cross-compilation). But that wouldn't solve the problem: we'd still need a build plan for them. The problem is that it's slow to make a build plan for build-tool-depends, and if we were to solve that problem for a hypothetical run-tool-depends, I don't see why we couldn't apply the same solution to build-tool-depends. Hence why I think it's better to just figure out why it's slow, and fix that (which is what's going on in #7472).

@Mikolaj
Copy link
Member

Mikolaj commented Aug 3, 2021

@Mikolaj can we prioritise this please?

@angerman: surely, let's prioritise the brainstorming and come up with a good design, while we fix related slowdowns in other tickets.

@newhoggy: thank you for fixing and clarifying the initial requirements.

@michaelpj: you are right, I missed that build-tool-depends in your repo --- apparently github only searches for whole words, because build-tool doesn't show any hits, while build-tool-depends does.

@gbaz
Copy link
Collaborator

gbaz commented Aug 3, 2021

Please bear with this rather circuitous description of an insight/idea I had:

In trying to think about this issue, I came at first to a conclusion similar to @michaelpj -- like, who's to say that solving for this is any faster than solving for build-tool-depends. Then I started thinking a bit more about what this would not do that the other does -- I suppose it would not introduce sub-components to solve for, but would instead just do the "plain" solving and only reorder the build-plan to ensure everything is built in an order respecting the constraints? But this raises the question -- why is sub-component solving slower than just plain solving? And the answer must be that the components get solved multiple times. But as per the discussion in the other ticket -- components are supposed to be linked so they get solved once and only once (if possible), right? So, my thought is that there might be something order dependent in if that linking actually saves time or not -- i.e. if B build-tool-depends on A, then solving A before B might attempt to reuse A, but solving B before A means solving B:A first, and that might not get reused for A?

Perhaps there's something quadratic or worse if we have a huge chain of build-tool-depends so we get A:B:C:D solved and also B:C:D solved and etc..? And somehow the order of solving prevents the linking from ensuring solver-reuse? @newhoggy does that correspond to how your repo works? @grayjay what do you think about that being perhaps at issue?

(Edit: Let me add the conclusion to this ramble. It seems that if we can isolate this quadratic issue then we can indeed solve it directly for build-tool-depends rather than adding a new clause to the cabal grammar.)

@newhoggy
Copy link
Author

newhoggy commented Aug 4, 2021

I did a grep to find chains of build-tool-depends in local dependencies and this is what I could find:

cardano-testnet.cabal:
   build-tool-depends:   cardano-node:cardano-node, cardano-cli:cardano-cli
cardano-node-chairman.cabal:
  build-tool-depends:   cardano-node:cardano-node, cardano-cli:cardano-cli, cardano-node-chairman:cardano-node-chairman
plutus-pab.cabal:
  build-tool-depends: cardano-node:cardano-node, cardano-cli:cardano-cli
plutus-tx.cabal:
  build-tool-depends: doctest:doctest
plutus-metatheory.cabal:
  build-tool-depends: plutus-core:plc, plutus-core:uplc
plutus-metatheory.cabal:
  build-tool-depends: plutus-core:plc, plutus-core:uplc
plutus-doc.cabal:
  build-tool-depends: doctest:doctest -any
plutus-core.cabal:
  build-tool-depends: alex:alex -any, happy:happy >=1.17.1
cardano-cli.cabal:
  build-tool-depends:   cardano-cli:cardano-cli

In the above, cardano-node-chairman depends on a bunch of cardano-* stuff. Those depend on cardano-api (not listed), which then depend on other stuff (also not listed) that ultimately depends on plutus-* stuff.

There could be hackage dependencies that do the same thing.

@newhoggy
Copy link
Author

newhoggy commented Aug 4, 2021

It seems that if we can isolate this quadratic issue then we can indeed solve it directly for build-tool-depends rather than adding a new clause to the cabal grammar.

Yep. If build-tool-depends performance is solved it would solve our biggest pain point, so in that sense I endorse @michaelpj 's comments.

The reason I raised the ticket is because I had the same intuition that adding an extra dependency link should not add any additional solving work because the component was already in the build plan. My model of how it could work is that we could solve as if the build-tool-depends wasn't there.

Those components would be included anyway because they are already a build target. Then post solving, when it comes time to build, in order to determine the partial order for building, the dependency graph could be updated to include build-tool-depends, which would delay, for example, the running of test:cardano-node-chairman until exe:cardano-node, exe:cardano-cli, exe:cardano-node-chairman are built.

There may be implementation details that make this impractical that I don't see because I don't actually know how cabal works. There are certainly some cabal behaviours I find surprising that suggests my model of how cabal works is not accurate.

@gbaz
Copy link
Collaborator

gbaz commented Aug 15, 2021

with the perf issues with build-tool-depends solving now resolved, can we close this ticket and just say "build-tool-depends should also be used for test component dependencies"?

@newhoggy
Copy link
Author

Thanks so much for resolving the solver performance issue!

@andreasabel
Copy link
Member

andreasabel commented Aug 26, 2021

with the perf issues with build-tool-depends solving now resolved, can we close this ticket and just say "build-tool-depends should also be used for test component dependencies"?

Link to the PR or commit that solved the performance issue?

Also, please put some informative tags on this issue.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 26, 2021

@andreasabel: mostly #7532, but also other PRs linked in the original ticket. What tags do you have in mind? Would you like to suggest some?

@andreasabel
Copy link
Member

What tags do you have in mind? Would you like to suggest some?

E.g. cmd/test.

@Mikolaj
Copy link
Member

Mikolaj commented Aug 26, 2021

Here you go. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants