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 tool-cache for Gleam and rebar3 #223

Merged
merged 14 commits into from
Aug 7, 2023
Merged

Feature tool-cache for Gleam and rebar3 #223

merged 14 commits into from
Aug 7, 2023

Conversation

paulo-ferraz-oliveira
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Jul 3, 2023

Description

This pull request:

  • in an attempt to ease maintenance...
    • gets rid of .ps1 and .sh files
    • moves all the content of src/installer.js into src/setup-beam.js
    • moves all the content of test/problem-matchers.test.js into test/setup-beam.test.js
    • centralises the mirror-iterating function in a single place, with a single interface (doWithMirrors)
      • while doing this we also ended up fixing an issue with not using mirrors for the Erlang/OTP build 👍
    • makes tool-cacheing -related behaviour callback -based (as explained in an extensive comment in the code)
    • improves debug logging
  • has Gleam and Rebar3 use tool-cache as per the issue it closes
  • is probably best reviewed commit by commit (given its size)
  • basically leaves the tests untouched in an attempt to increase confidence on the changes
    * ⚠️ is not yet to be merged
    * I want to get Rework our version matching algorithm #217 approved+merged so as to update this one (I'm sure there'll be merge conflicts)

Closes #204.

Callback -based tool-cacheing -related behaviour

As an example, we show how Gleam is now configured for tool caching, on Linux (simplified here for illustration purposes):

downloadToolURL: () =>
  `https://github.com/gleam-lang/gleam/releases/download/${versionSpec}/gleam-${versionSpec}-x86_64-unknown-linux-musl.tar.gz`,
extract: async (file) =>
  ['dir', await tc.extractTar(file, undefined, ['zx'])],
postExtract: async () =>
  {},
reportVersion: () =>
  ['gleam', ['--version']]

Comment on lines -113 to +141
got = await setupBeam.getOTPVersion(spec, osVersion, hexMirrors)
got = await setupBeam.getOTPVersion(spec, osVersion)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think hexMirrors was just exposed for testing purposes, which is not required, since we have a way to simulate inputs.

Comment on lines -61 to +65
await installer.installOTP(osVersion, otpVersion, hexMirrors)
await doWithMirrors({
hexMirrors: hexMirrorsInput(),
actionTitle: `install Erlang/OTP ${otpVersion}`,
action: async (hexMirror) => {
await install('otp', {
osVersion,
toolVersion: otpVersion,
hexMirror,
})
},
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is what we did to replace input hexMirrors with internal function iteration: we declare a function that implements the iteration internally (which hopefully eases maintenance and makes for more consistent behaviour, when required).

Comment on lines +637 to +704
// The installOpts object is composed of supported processPlatform keys
// (e.g. 'linux', 'win32', or 'all' - in case there's no distinction between platforms)
// In each of these keys there's an object with keys:
// * downloadToolURL
// - where to fetch the downloadable from
// * extract
// - if the downloadable is compressed: how to extract it
// - return ['dir', targetDir]
// - if the downloadable is not compressed: a filename.ext you want to cache it under
// - return ['file', filenameWithExt]
// * postExtract
// - stuff to execute outside the cache scope (just after it's created)
// * reportVersion
/// - configuration elements on how to output the tool version, post-install
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This (in a gist) explains the major change behind this pull request.

If it's not clear (since this is supposed to explain the "behaviour" required for adding new tool-cached stuff) it can be improved (variables renamed, behaviour made more explicit, etc.).

Comment on lines +872 to +945
async function installTool(opts) {
const { toolName, versionSpec, installOpts } = opts
const platformOpts = installOpts[process.platform] || installOpts.all
let cachePath = tc.find(toolName, versionSpec)

core.debug(`Checking if ${installOpts.tool} is already cached...`)
if (cachePath === '') {
core.debug(" ... it isn't!")
const downloadToolURL = platformOpts.downloadToolURL()
const file = await tc.downloadTool(downloadToolURL)
const [targetElemType, targetElem] = await platformOpts.extract(file)

if (targetElemType === 'dir') {
cachePath = await tc.cacheDir(targetElem, toolName, versionSpec)
} else if (targetElemType === 'file') {
cachePath = await tc.cacheFile(file, targetElem, toolName, versionSpec)
}
} else {
core.debug(` ... it is, at ${cachePath}`)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Along with the comment block shown above, this is the function that implements the generic "behaviour" used, at this moment, for 4 tools in 2 OS types. This has been tested with the addition of Gleam and Rebar3 to tool-cache and retrofitted for Erlang/OTP and Elixir.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

Force-pushing to resolve conflicts. Flagging this as "Ready for review", too...

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as ready for review July 21, 2023 20:15
At the same time we fix an error, where we aren't using the
mirrors for the OTP build :)
This hopefully eases introduction of new languages at the cost
of a bit of abstraction (there's an extensive comment on how to
fill in the install options' object)

We'll soon test this abstraction by making Gleam and rebar3
cached too
Also, gets rid of all .sh
Also, gets rid of all .ps1
And also be consistent when it comes to using
fs....Sync vs fs.promise....
@paulo-ferraz-oliveira
Copy link
Collaborator Author

Rebased on top of main.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

ping @starbelly

Copy link
Member

@starbelly starbelly left a comment

Choose a reason for hiding this comment

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

This was a beautiful PR. LGTM! 👏 👏 👏

Let's delay the release but one day or so and ask others to test main. I will do so tomorrow myself in various projects.

@paulo-ferraz-oliveira paulo-ferraz-oliveira merged commit afb8586 into erlef:main Aug 7, 2023
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/cache-for-gleam-and-rebar3 branch August 7, 2023 09:32
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.

Use 'action/tool-cache' for Gleam and rebar3
2 participants