Skip to content

Conversation

jchavarri
Copy link

@jchavarri
Copy link
Author

@jordwalke @ulrikstrid this seems to be ready. I fixed a related issue where esy install was being called before restoring the cache.

Build times are
Linux: 1m 57s
macOS: 1m 52s
Win: 4m 18s

(don't take last build into account as I removed a string from the cache key string which invalidated it).

@ulrikstrid ulrikstrid mentioned this pull request Dec 2, 2019
- powershell: esy.cmd import-dependencies
continueOnError: true
condition: and(eq(variables['AGENT.OS'], 'Windows_NT'), and(eq(variables['Build.Reason'], 'PullRequest'), and(succeeded(), ne(variables['Build.SourceBranch'], variables['System.PullRequest.TargetBranch']))))
Copy link
Contributor

Choose a reason for hiding this comment

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

This stuff was important (esy import-dependencies). Is it moved somewhere else?

Copy link
Contributor

Choose a reason for hiding this comment

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

What happened was Azure changed the location of where our cache directory was on the physical file system, and so artifacts broke in future builds because the artifacts had hard coded paths in them. So I made sure that we actually ran esy export-dependencies and esy import-dependencies which fixes that issue.

(We can also use the REST API locally from laptops to pull down those caches and warm up our local development).

Copy link
Author

Choose a reason for hiding this comment

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

@jordwalke Hm... I didn't realize this was added because of that. The path issue does not seem to be happening now (e.g. https://dev.azure.com/esy-ocaml/esy-ocaml/_build/results?buildId=421). Do you remember if this issue happened in specific situations or how could we repro it?

Also, did it happen in all platforms? I was wondering if maybe the recent upgrade you did to Windows_2019 vm would have fixed that problem.

We can also use the REST API locally from laptops to pull down those caches and warm up our local development

This is definitely a regression. I wonder if there is a way to expose the cache in the release artifacts? Reintroducing the REST API based flow would mean we would have to own again the issues with permissions, source and target branch handling etc which was one of the nice upsides of using Azure cache.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't realize this was added because of that. The path issue does not seem to be happening now (e.g. https://dev.azure.com/esy-ocaml/esy-ocaml/_build/results?buildId=421). Do you remember if this issue happened in specific situations or how could we repro it?

Some of the paths unexpectedly changed (the user directory where we have the esy cache for example). I don't believe they make any promises that it will not change unexpectedly. So I put the esy import/export in there because it guards against it ever changing in the future - as well as enabling the ability for us to use the cache to pull down artifacts from CI to warm up our laptops (it can't be done without first esy export-dependencies)

Copy link
Contributor

Choose a reason for hiding this comment

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

We can also use the REST API locally from laptops to pull down those caches and warm up our local development

This is definitely a regression. I wonder if there is a way to expose the cache in the release artifacts? Reintroducing the REST API based flow would mean we would have to own again the issues with permissions, source and target branch handling etc which was one of the nice upsides of using Azure cache.

I think I might not have been clear. What I was saying was that if the cache is in esy export-dependencies form, then it is trivial for local development machines (laptops) to have simple bash scripts that use the REST API and pull them down and magically speed up their local builds. (This is most important for Windows).
I wasn't suggesting that the only way to get the caches in esy export-dependencies form is to use the REST API. It's just that the .ci configurations have diverged and since the new Azure cache started being prototyped outside of this repo, I had already improved some of the steps in this repo's CI config, and then they got out of sync in terms of their features. So I'm asking if this PR can be updated to include the features that I added in this repo, before we merge?

Using esy import-dependencies and esy export-dependencies were the two that I could think of (I also added a couple of custom steps for windows which delete the package ocamlfind from the imported build cache (which we need until my PR for esy lands)).

I had to configure the esy import-dependencies a specific way (note my previous config which calls esy.cmd). It wouldn't work otherwise.

continueOnError: true
condition: and(eq(variables['AGENT.OS'], 'Windows_NT'), and(eq(variables['Build.Reason'], 'PullRequest'), and(succeeded(), ne(variables['Build.SourceBranch'], variables['System.PullRequest.TargetBranch']))))
displayName: "Print env in powershell"
# Needed so that the mingw tar doesn't shadow the system tar. See
Copy link
Contributor

Choose a reason for hiding this comment

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

Also note that this here was required when running export-dependencies and import-dependencies because we can't have mingw tar shadowing the windows tar that we need.

@jchavarri
Copy link
Author

@jordwalke I tried to re-add the import- / export-dependencies commands.

I am getting two sorts of errors:

  1. on esy import-dependencies, I get some errors of this kind
esy: internal error, uncaught exception:
     Unix.Unix_error(Unix.EACCES, "rename", "C:\\Users\\VssAdministrator\\.esy\\3_\\s\\reason_native__s__pastel-0.2.1-0d140e57")
     Raised at file "src/core/lwt.ml", line 2998, characters 20-29
     Called from file "src/unix/lwt_main.ml", line 26, characters 8-18
     Called from file "esy-lib/Cli.re", line 263, characters 9-28
     Called from file "cmdliner_term.ml", line 25, characters 19-24
     Called from file "cmdliner.ml", line 25, characters 27-34
     Called from file "cmdliner.ml", line 116, characters 32-39

They happen with different libraries: rely, pastel, console in particular.

  1. Then on esy build I am getting errors like:
info esy build 0.5.8 (using package.json)
info building [email protected]@d41d8cd9
error: build failed with exit code: 1
  build log:
    # esy-build-package: building: [email protected]
    # esy-build-package: pwd: C:\Users\VssAdministrator\.esy\3_\b\ocaml-4.8.1000-a241290c
    esy-build-package: delete directory
                       C:\Users\VssAdministrator\.esy\3_\s\ocaml-4.8.1000-a241290c:
                       C:\Users\VssAdministrator\.esy\3_\s\ocaml-4.8.1000-a241290c\lib\ocaml:
                       Directory not empty

The latest build includes both cases.

Something I realized is that the ocamlfind cleanup is being done after calling esy import-dependencies in master branch, which is confusing as it's supposed to remove ocamlfind before calling that command? 🤔

@jordwalke
Copy link
Contributor

jordwalke commented Dec 10, 2019

It's supposed to remove ocamlfind after importing. There's a problem with importing ocamlfind that my (now merged) PR fixes. We might want to see if esy nightlies fix it (https://www.npmjs.com/package/@esy-nightly/esy) now that my PR is merged. With my PR, we don't need to delete ocamlfind (though I haven't tried yet). I could look at it next weekend if you don't get to it.

@ManasJayanth
Copy link
Contributor

@jchavarri I was just reading about Cache@2 docs. This PR looks really cool, and I'm happy to help. Let me know

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.

4 participants