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

Post-install integration tests [with default install] #4099

Open
mgree opened this issue Feb 24, 2020 · 13 comments
Open

Post-install integration tests [with default install] #4099

mgree opened this issue Feb 24, 2020 · 13 comments

Comments

@mgree
Copy link

mgree commented Feb 24, 2020

I've recently run into two separate issues with OPAM packages (my own libdash as well as the ever-popular z3) which would have been avoided if OPAM supported integration tests.

The issue is that OPAM's test targets seem to be run before installation happens. Not only does this present a challenging linking problem for libraries that make use of C code (since now you must accommodate two different linking scenarios---in .opam and also in the build directory), it also means that it's hard to verify that the installed library will link correctly with clients.

The feature could be quite simple: add a post-install-test (or some similar name) top-level build field that contains scripts in the same style as build, install, run-test. These scripts would only be run when --with-test was used a flag; they would be run only after a successful install. If the tests failed, I can imagine either (a) simply reporting the failure to the user, or (b) uninstalling the software.

@rjbou
Copy link
Collaborator

rjbou commented Feb 25, 2020

it is already possible to express it. You can add in the install: field, in the end, {with-test} instructions, that would be run then after install ones only if --with-test is given.
Related issue #4069

@mgree
Copy link
Author

mgree commented Feb 25, 2020

I see, thank you. This didn't work for me in earlier builds, but perhaps for unrelated reasons.

My current install script in OPAM now looks like:

install: [
  ["opam-installer" "--prefix=%{prefix}%" "libdash.install"]
  [make "-C" "test" "test"] {with-test}
]

Is there a better option for that first line? NB that libdash.install is generated dynamically.

@mgree
Copy link
Author

mgree commented Feb 25, 2020

It must not be the preferred way---I'm getting a failure in my tests. Any advice?

@mgree
Copy link
Author

mgree commented Feb 25, 2020

I've also tried installing using ocamlfind, which also failed (it didn't install symlinks, which are necessary for the shared libraries). I've tried having my library depend on opam-installer, which fails because opam-format=2.0.6 won't install.

I'd really appreciate some guidance here. Is there some utterance I can give in the install clause that means "please do the default thing with <package>.install"?

@rjbou
Copy link
Collaborator

rjbou commented Feb 26, 2020

No, its the default in case there is no instructions in the install: field, but here as you only have a check, it will work on normal install, but not with tests. We must indeed handle such case, well spotted!

For your package, to have something working for the moment, you can add opam-installer and its corresponding instruction with a with-test condition, like that, it will be installed, run only on a test enabled run.
Or even have a script that does the install, as you generate via a script libdash.install. For prefixes, you can have in the scripts opam variables and add it in a substs: field.

[moved in the accurate issue opam-format comment]

@rjbou rjbou changed the title Post-install integration tests Post-install integration tests [with default install] Feb 26, 2020
@mgree
Copy link
Author

mgree commented Feb 26, 2020

I see. I haven't had success with using ocamlfind, but opam-installer should work (once #4100 is resolved)... or even just cp, though then I'm no longer confident that .opam-switch/install/<package>.install will be correct. (Does the sandbox automatically detect and log such copies, or... ?

In any case, I'm more or less content with the current setup and need to pay attention to other things. I'd of course prefer to run integration tests after the install, so I'll update things when I can. My current setup is to do that in the same Docker container that tests the package itself.

Longer term, I think it'd be great to (a) add an action in the install: section of OPAM files that looks for <package>.install and runs it, and (b) clarifies the docs about how that's the default install: section and how you can use it.

@rjbou
Copy link
Collaborator

rjbou commented Feb 27, 2020

You can use install instead of cp. What do you mean by you're not confident that install file will be correct ? You generate it via a script, no ?

For tracking, it is not the sandbox that does this work, but ad directory tracking is done before and after install. The install file is also kept, so removal operations are taken from the install file if present, and also from directory tracking result.

If you have the script libdash_install.sh that installs the files, you can run the tests afterwards.

on install: documentation, there is already a paragraph on .install files. Maybe worth adding more explicitly that to run test after install, you must have at least an install item (and also fix the test with automatic install issue).

@mgree
Copy link
Author

mgree commented Feb 27, 2020

I'm confident that I can generate a correct libdash.install. I was worried that if I used cp that the file record in .opam might be tracked correctly. It looks like that would be tracked in, e.g., libdash.changes, right?

I'm not sure I follow you in the beginning. Is there any documentation for the install command? Is it like the make command?

Finally, I just noticed post-install-commands, which almost works for me (except I wouldn't want to run those commands when installation had failed). One possibility might be:

post-install-commands: [
  [make "-C" "tests" "test" {with-test & libdash:installed}]
]

If that seems reasonable, I'll try it.

@rjbou
Copy link
Collaborator

rjbou commented Feb 27, 2020

Yes, changes occurring at install action are all tracked, to be sure to clean everything (in the limit that the install is in the opam switch directory, sandbox ensures that.

On the install command, maybe I wasn't clear, I was talking about the linux command that does a cp and sets the good attributes. Cf. its [manpage]((https://linux.die.net/man/1/install).

The post-install-commands: field is not an opamfile field but a configuration one (switch, global, opamrc). If you set it up in your config, it will be called for each package install, and in this case once libdash has been installed. Besides, note that a package is marked as installed only when all install action ended well, i.e. at post-install-commads: stage, it is not yet marked at installed.

@mgree
Copy link
Author

mgree commented Feb 27, 2020

Oh: that install. I see, gotcha. And: sad news about post-install-commands:.

Would it be helpful if I proposed extensions to handle the "default actions + extra" case?

@rjbou
Copy link
Collaborator

rjbou commented Mar 3, 2020

It would help for sure, all contributions are welcome :)

First we need to discuss about it, because it have several implications. We can't always install the install file even if there is install instructions. I see two ways to do it:

  • check if the only command is a test command → we need to propagate the information to the install file condition
  • add an install variable, as make that is simply resolved as install the generated install file. We would have then
install: [
  [ install ]
  [ make "tests" ] {with-tests}
]

@mgree
Copy link
Author

mgree commented Mar 4, 2020

I quite like the latter approach, since it's far more flexible. The only downside I can see to the syntax you proposed is that it's confusingly similar to scripts that might use the BSD install tool, i.e.,

install: [
  [ "install" "foo.cmxa" "%{_:lib}%" ]
]

Maybe change it to default-install?

@rjbou
Copy link
Collaborator

rjbou commented Mar 4, 2020

yes, it can be default-install to not set a confusion.

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

2 participants