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

Add a reproduction case for issue #4429 #4430

Merged
4 commits merged into from
Apr 1, 2021
Merged

Add a reproduction case for issue #4429 #4430

4 commits merged into from
Apr 1, 2021

Conversation

craigfe
Copy link
Contributor

@craigfe craigfe commented Mar 31, 2021

No description provided.

@ghost
Copy link

ghost commented Mar 31, 2021

Thanks for the repro case. Could you use dune-build-info instead of the cache to reproduce this bug? @snowleopard is doing some restructuring of the code for the shared cache and the cache will stop calling git. So this repro case will soon stop working.

@snowleopard
Copy link
Collaborator

Thanks for the repro case. Could you use dune-build-info instead of the cache to reproduce this bug? @snowleopard is doing some restructuring of the code for the shared cache and the cache will stop calling git. So this repro case will soon stop working.

Thanks, indeed.

@craigfe After some upcoming changes, Dune cache will no longer require VCS information. The only reason it currently needs it is to support bulk download of build artifacts corresponding to a given commit but we found that in practice downloading artifacts for individual rules is simpler and fast enough.

@craigfe
Copy link
Contributor Author

craigfe commented Apr 1, 2021

Thanks both. I've altered the reproduction to avoid Dune caching.

I wasn't able to get a reproduction w/ dune-build-info. Interestingly, the following works just fine:

  Initialized empty Git repository in $TESTCASE_ROOT/.git/
  $ echo "(executable (name main) (libraries dune-build-info))" > dune
  $ cat >main.ml <<EOF
  > ;; Printf.printf "internal version: %s\n"
  > ( match Build_info.V1.version () with
  > | None -> "None"
  > | Some v -> Build_info.V1.Version.to_string v )
  > EOF
  $ dune exec ./main.exe
  Info: Creating file dune-project with this contents:
  | (lang dune 2.8)
  internal version: None

(Probably just me missing something obvious.) I had a quick look through the internals, but couldn't work out why the emitted vcs-describe placeholder isn't translated into a failing Git command somewhere. Will take a deeper look at some point when I get the time; dune-build-info seems a more compelling reproduction than dune subst.

@ghost
Copy link

ghost commented Apr 1, 2021

It's because we don't include the actual info when building executables in the _build directory. This is so that dune-build-info doesn't hurt incrementality, otherwise we'd end up rebuilding a bunch of stuff everytime we do a commit. There are two ways of getting the actual info:

  • with dune install
  • by adding (promote (until-clean)) to the executable stanza to make dune promote the exe

craigfe added 2 commits April 1, 2021 11:59
Signed-off-by: Craig Ferguson <me@craigfe.io>
Signed-off-by: Craig Ferguson <me@craigfe.io>
@craigfe
Copy link
Contributor Author

craigfe commented Apr 1, 2021

Thanks for the explanation :-) Now using dune-build-info properly.

_
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@ghost ghost merged commit e8ad49f into ocaml:main Apr 1, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants