-
Notifications
You must be signed in to change notification settings - Fork 698
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
Install test executables with cabal install when "--enable-tests" present #7693
Comments
This is intended: cabal/cabal-install/src/Distribution/Client/CmdInstall.hs Lines 408 to 416 in 103adec
It was introduced in #4879 to fix #4855. Things might have changed since then though (eg. #4886 (comment)), judging by eg. #5143 |
To spell it out, you did |
I want to build/install Agda, and run the tests. If I do
It would be great if this obstacle were taken care of. |
I just tried removing cabal/cabal-install/src/Distribution/Client/CmdInstall.hs Lines 408 to 416 in 103adec
and there is no assertion failure, though tests aren't actually built. Maybe we just need the equivalent of #5143. I won't investigate this further right now though |
So why do you do
I hope that's not true (otherwise, please report). I you |
unfortunately it's true for install: #6919 |
The example from #6919 is very sad but, if you squint, it's excluded by the "with the same flags" proviso (targets being not exactly flags, but being at least commandline arguments). And I've heard that stuff gets rebuilt without a good reason in other cases, which is why I'm fishing for a reproducible example. |
It's still true even with the same flags: when installing, local packages are treated as remote and have to be rebuilt in the store |
So the "I notice that, if I |
Strictly speaking, those are different flags, albeit slightly. As I understand (please correct me if I'm wrong), That's not unique to Haskell/Cabal - in C people debug and test with
To revisit this question - no, I don't really want to run tests on/for the dependencies - but these dependencies must be ready to support running tests on my project's target. I suspect, in the end there isn't much difference between whether I want to actually run tests for the dependencies, or merely build and install those dependencies in such a way that they can support their caller running tests. Thus, I think that the correct behavior for Cabal in such a case should be - build the dependencies as if I were to run the tests for them, regardless of whether they are local or remote. Comments, please? |
I don't even understand the --only-dependencies flag here. It doesn't make a lot of sense in this workflow given how v-2 works. I think the flags are asking for something a bit nonsensical and the current behavior looks fine. I think it is correct that install does not allow tests, since it doesn't make much sense. To get the workflow you desire, you can just build with tests, then manually copy or symlink the binary. A future feature request could be to add a copy/symlink binary flag to the build of an executable. |
IMHO, on the contrary - it makes great sense to be able to run the test suite over the "production" (aka, installed) executable... At the very least, you'll know that the actual physical binary that passed or failed your tests is the same that is running your high-value tasks in deployment. What am I missing? |
@mouse07410: I'm pretty sure Regarding the ability to test exactly the thing you install, I think it makes sense, except that the installed executable would never be tested, because it's the libraries that are linked with the test component, executables are normally not used for tests. Generally, test component in cabal are rather unit tests for the code, while executables are tested using integration and similar tests running from the outside. |
As I understand, both of the above mean - the binaries will be different. For an ideally simple code it may not matter - but I haven't seen such a code in real life.
"Never say Never again!" ;-) |
OK, you are right, in practice the compiled dependencies will be different. Moreover, if I wanted to test them afterwards, less would need to be rebuilt. However, I'm clarifying all these things in order to understand why you put
Haha, sure, but we need to focus on a particular use case. We don't have the resources to support everything. And we support only unit tests (more precisely, library tests) in cabal test components. Anybody using it to run integration tests (invoke an executable compiled in the same package) is on his own. Does it make sense? |
Hmm that sentence surprises me, f.e. in hls we have a few unit tests and lot, lot of integration tests in test stanzas which call the hls executable. EDIT: i guess the sentence would make sense in the context of |
I may be wrong. But if I'm not, that means you are on your own. E.g., ensuring that the |
If I did that, I'd probably have a semi-reasonable explanation. Since it's a part of the installation setup of a large (too large for my comfort) Agda compiler that I'm only a user of (not a developer or a maintainer) - I've no clue why that argument is there. My layman's common sense suggests that it doesn't really belong, but... I asked the question on the Agda repo (agda/agda#4216 (comment)), let's see what the answer is. |
to workaround the (again) recompilation with |
I looked at the agda ticket. They use --only-dependencies with the build command, not the install command. Asking to install with --enable-tests set doesn't seem important to the use case they have. Similarly, they use --enable-tests with the build command, not the install command. I think this ticket is just confusing, and confused. Its not addressing a real world case. |
I don't think this is right, because with Cabal their build process does not use The ticket talks about various (failed) experiments.
I don't know what you're basing this conclusion on. I say they do use Their installation from the source (that includes unit and integration tests) is designed to run from Makefile. Here's what the successful installation log shows (I'd be happy to post the entire log, if anybody needs it):
You can see two Again, here are just the "relevant" lines:
The Agda ticket in question is not crisp (to say the least), as it tackles (mixes) several issues. Some of them have been long addressed, like, using |
@mouse07410, I haven't looked at the Agda issue, so I can't comment about that, but just a remark that v1-install corresponds to v2-build. And v2-install is a completely different beast and in flux. Edit: and we asked why you use |
tbh I think the main culprit is v2-install recompile local packages after a v2-build, so fixing that adding a copy-bins to v2-build would make less desirable the original request |
Yes, but... In my case, all the builds are dynamic. It implies that copying just the executables won't be enough - all the created binaries (executables and libraries) have to be copied with Along the same line, what would be the difference between |
Sorry, I do not understand. To me:
What am I missing? |
Incorrect.
Incorrect.
Almost correct, except that |
Well, yes - but the fact that
Yes - but, as I said above, "globally available and runnable" requires that all of the dependencies of the current targets and all of the dynamic libraries of the current targets are also installed globally. So, I'm not sure what you meant by "nothing else", as AFAIK, there is nothing else.
I mean - the goal of the workflow that the developers of Agda compiler set up, and which I'm not competent enough to modify. On the other hand, that workflow makes a lot of sense - if my package pulls a lot of dependencies, I do not want to have to recompile them all every time I tweak something in my package, especially if that tweak is only "minor" flags.
I'm reasonably sure I did try that, and it recompiled everything when it came to building the "main" package (Agda itself). In addition to the listed problems, there were issues with |
Seriously, are we disputing whether my use of "normal" was justified? |
No, not really. ;-) The question is whether the current Cabal supports the following workflow, and if it does - how to do it:
|
Cabal supports all but the last step, where you can copy binaries, but it will not copy shared libraries. The alternate workflow, where one builds twice, once a build tests, and once with the "install" command, to install, will still use all the existing installed dependencies in the global store. This seems a minor inconvenience, at most. I will repeat myself very clearly: despite the older (v1) agda instructions, installing with tests does not seem a workflow we should concern ourselves with supporting. |
Will this alternate workflow rebuild the dependencies in the global store? If not - then there's no inconvenience, as far as I'm concerned. What should the exact commands be then for these two builds (assuming one wants to only use
I really don't want Cabal to recompile all the dependencies just because |
So in fact, assuming you don't actually want to run tests, then a single call to |
Thank you! Much clearer. So, in your opinion I don't even need A silly question: at the last |
No. All the enable-tests flag does is configure the package to build the test suite. But the test suite is, by design, not installed. So it makes no sense to enable tests when installing. |
Sorry, no good. Here's the build command:
Here's the install command:
You can see what remote packages it thinks it needs to build:
And you can see that the very same remote packages are now being rebuilt:
The only difference in flags is absence of Here are the logs: $ time cabal v2-install --disable-documentation -foptimise-heavily -fenable-cluster-counting --ghc-options="+RTS -M6G -RTS" --program-suffix=-2.6.3 2>&1 | tee cabal-install-out.txt
Wrote tarball sdist to
/Users/ur20980/src/agda/dist-newstyle/sdist/Agda-2.6.3.tar.gz
Resolving dependencies...
Build profile: -w ghc-9.0.1 -O1
In order, the following will be built (use -v for more details):
- hashable-1.3.4.1 (lib) (requires build)
- time-compat-1.9.6.1 (lib) (requires build)
- these-1.1.1.1 (lib) (requires build)
- async-2.2.4 (lib) (requires build)
- scientific-0.3.7.0 (lib) (requires build)
- unordered-containers-0.2.14.0 (lib) (requires build)
- hashtables-1.2.4.1 (lib) (requires build)
- data-fix-0.3.2 (lib) (requires build)
- uuid-types-1.0.5 (lib) (requires build)
- case-insensitive-1.2.1.0 (lib) (requires build)
- strict-0.4.0.1 (lib) (requires build)
- attoparsec-0.14.1 (lib) (requires build)
- aeson-1.5.6.0 (lib) (requires build)
- Agda-2.6.3 (exe:agda, exe:agda-mode) (requires build)
Starting hashable-1.3.4.1 (lib)
Building hashable-1.3.4.1 (lib)
Installing hashable-1.3.4.1 (lib)
Completed hashable-1.3.4.1 (lib)
Starting these-1.1.1.1 (lib)
Starting scientific-0.3.7.0 (lib)
Starting unordered-containers-0.2.14.0 (lib)
Starting data-fix-0.3.2 (lib)
Starting async-2.2.4 (lib)
Starting uuid-types-1.0.5 (lib)
Starting case-insensitive-1.2.1.0 (lib)
Starting time-compat-1.9.6.1 (lib)
Starting hashtables-1.2.4.1 (lib)
Building data-fix-0.3.2 (lib)
Building unordered-containers-0.2.14.0 (lib)
Building scientific-0.3.7.0 (lib)
Building these-1.1.1.1 (lib)
Building time-compat-1.9.6.1 (lib)
Building async-2.2.4 (lib)
Building uuid-types-1.0.5 (lib)
Building case-insensitive-1.2.1.0 (lib)
Building hashtables-1.2.4.1 (lib)
Installing data-fix-0.3.2 (lib)
Installing async-2.2.4 (lib)
Installing case-insensitive-1.2.1.0 (lib)
Completed data-fix-0.3.2 (lib)
Installing uuid-types-1.0.5 (lib)
Completed async-2.2.4 (lib)
Installing these-1.1.1.1 (lib)
Completed case-insensitive-1.2.1.0 (lib)
Completed uuid-types-1.0.5 (lib)
Completed these-1.1.1.1 (lib)
Starting strict-0.4.0.1 (lib)
Installing scientific-0.3.7.0 (lib)
Installing time-compat-1.9.6.1 (lib)
Completed scientific-0.3.7.0 (lib)
Starting attoparsec-0.14.1 (lib)
Completed time-compat-1.9.6.1 (lib)
Building strict-0.4.0.1 (lib)
Building attoparsec-0.14.1 (lib)
Installing unordered-containers-0.2.14.0 (lib)
Installing hashtables-1.2.4.1 (lib)
Completed unordered-containers-0.2.14.0 (lib)
Completed hashtables-1.2.4.1 (lib)
Installing strict-0.4.0.1 (lib)
Completed strict-0.4.0.1 (lib)
Installing attoparsec-0.14.1 (lib)
Completed attoparsec-0.14.1 (lib)
Starting aeson-1.5.6.0 (lib)
Building aeson-1.5.6.0 (lib)
Installing aeson-1.5.6.0 (lib)
Completed aeson-1.5.6.0 (lib)
Starting Agda-2.6.3 (all, legacy fallback)
Building Agda-2.6.3 (all, legacy fallback)
. . . . . [build-for-install is still running] |
@mouse07410 I am with you, each time I have to install hls I have to wait for two hours to see the end of the build See #7297 (comment) for more info about the origin of build Vs install differences. So from a user point of view the objective would be avoid hard to do rebuilds, no matter the underlying impl would be. That said, don't you get a rebuild of local packages when you switch enable-tests between two |
Fur me as a user is a big inconvenience. For a big-project developer that uses CI it's a disaster, because the time CI allows the build+run to consume gets eaten up by this stupid rebuild. In the end, making it impossible to use CI. Not a good thing!
That's the whole point. I can understand why you don't want to install the tests you built. I do not understand why I cannot install the executables (and libraries) that were built with I think that upon Or am I missing something? |
Please let's use #6919 to discuss whether
I cannot reproduce this. In the insall command, all remote packages are reused. Maybe there is a difference deep in the dependency trees of the two build plans you're getting that causes all that (justified in that case) recompilation. You could check that with cabal-plan. |
I'm pretty sure that the complexity of the Agda project build contributes a lot to exposing this problem - but I say it's still a Cabal problem.
Not sure I understand what you mean - but
And, of course
UpdateOne more concern:
The executable depends on the project-built shared library
|
Just tried this (though with ghc 8.10.4) and can't reproduce the deps being rebuilt when trying an install after a build with --enable-tests. Note that it appears only a small subset of libs are being rebuilt, so its not a general issue, but something pertaining to those libs in particular. This may have to do with the dynamic linking opts in your ~/cabal.config |
A sure reproducer for me in Agda repo itself - a bit on the big side, but should suffice as a proof... All of my toy projects are trivially small, so not representative...
Or, rather, a general issue impacting specific conditions. I doubt it's specific to individual libs/packages - probably, specific to what flags their project files define...?
No doubt, dynamic linking exacerbates the problem. Though I doubt it causes it. |
I literally tried this with the agda repo. I couldn't reproduce. |
Same and same.
if you run |
My apologies for being dense. Say, I have a pristine/clean clone of the repo, ready for |
So not sure if my test is correct but i tried a raw List of dependencies
This is the cabal plan after cabal-plan output
cabal plan after cabal-plan output
And finally the output of cabal-plan in the temp dir used for build by Output of `cabal-plan info --builddir C:\TEMP\cabal-install.-19884\dist-newstyle`
EDIT:
|
Edit: apparently, the below doesn't make sense apart of pointing out the connection to #7297, see two comments down. . @jneira: I do not claim to understand this, but @phadej says it's expected and hard to avoid that unit IDs differ between build and install: #7297 (comment) Given that, I guess it's unavoidable that rebuilds occur. Perhaps we need to dig deeper into the IDs and/or #7297. Sanity check: the unit IDs in the plans you attached do differ, right? |
@Mikolaj I think you misinterpreted oleg's comment. The point is that if I "install" or "build" a package, its own hash may be different in those two circumstances. In either case, the packages it depends on will unilaterally be installed into the store. So the differences he's highlighting are not the relevant ones. @mouse07410 please keep your comments here constructive. Stomping your foot and repeating yourself, saying "that's a bug that needs fixing" while we're still trying to investigate what the reported issue even is, and if we can reproduce it is just adding noise and confusion that's making sorting this out harder. We know what your intuition is. The complicated reality of the system may not conform to that, and maybe the result will be fixing a bug, maybe it will be us understanding and documenting the system better, or maybe it will be realizing that a change could be possible but would require far too much work to be feasible. We don't know, and what we need to do now is patiently investigate, not repeat the same initial concerns. |
Well, there is one relevant case: if a remote package depends on a local package, the remote package and all its reverse dependencies will be rebuilt. This can happen if one vendors a package deep in the dependency tree. I see no such thing in the Agda repo though.
this should work but i did not test it: cabal build ...options...
cabal install --builddir=dist-newstyle-2 ...options...
cabal-plan diff --builddir dist-newstyle --builddir dist-newstyle-2 see |
I'm afraid you lost me here - I don't see how it could be possible for a dependency package (presumably already on Hackage) to depend in turn on the (presumably local) package being currently built. Most likely, I just don't understand the example.
Well, in my case, since I've already tried both
|
Ok, i think we are out of the issue topic now, it asks for installing executables with EDIT: I ended to open a new issue #7745, as #6919 is about rebuilding local packages and in my case cabal rebuilds all the deps too |
Describe the bug
Trying to manually (because included
Makefile
forcesv1-
commands) build Agda from source, I'm getting the following error:To Reproduce
Steps to reproduce the behavior:
Expected behavior
Successful build. I am getting it when I omit
--enable-tests
, but in that caseagda-tests
executable is not built (so, I can't run tests to validate the created executables).Note, that
cabal v2-build --enable-tests
seems to work fine (but it does not install the executables it builds).System information
cabal-3.6.0.0
,ghc-9.0.1
The text was updated successfully, but these errors were encountered: