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

Get the minimal flavor from the ci-artifacts GitHub Release #965

Merged
merged 4 commits into from
Dec 17, 2024

Conversation

dennisameling
Copy link
Contributor

As a result of recent changes, the Git SDK ci-artifacts are published as GitHub release assets now. This eliminates the need for us to clone and cache the artifacts in this GitHub action.

Let's simply download the latest .tar.gz from the ci-artifacts and extract it.

Ref: git-for-windows/git-sdk-64@fdb0cea
Ref: git-for-windows/git-sdk-64#87

@dennisameling dennisameling force-pushed the minimal-flavor-from-ci-artifacts branch from 9c351a5 to 5a3cf09 Compare September 22, 2024 17:37
@dennisameling
Copy link
Contributor Author

dennisameling commented Sep 22, 2024

Some initial results:

  • Running the action now takes only 3s, down from 2m43s (uncached)!!!
  • Triggered a test build of git-for-windows/git here, awaiting results and it seems to be working as expected.

dennisameling and others added 4 commits December 17, 2024 17:43
As a result of recent changes, the Git SDK `ci-artifacts` are published
as GitHub release assets now, including several variants of the minimal
flavor of Git for Windows' SDK. This eliminates the need for us to clone
the minimal SDK in this GitHub Action.

Let's simply download the latest `.tar.gz` from the `ci-artifacts` and
extract it.

Note: As per the analysis in
git-for-windows/git-sdk-64@fdb0cea37389, we
use the native `tar.exe` to unpack the minimal SDK while it is
downloaded, for maximal speed.

The analysis also suggests to use the `.zip` file, as this results in
the fastest operation when download speeds are above 6MB/second (which
we hope will be reliably the case in GitHub Actions). However, we want
to pipe the archive to `tar -xf -` while fetching, and it seems that for
some reason `C:\Windows\system32\tar.exe` misses `.sparse/` and `etc/`
when extracting `.zip` files from `stdin`, but the same is not true with
`.tar.gz` files. So let's use the latter.

See also: git-for-windows/git-sdk-64#87.

Signed-off-by: Dennis Ameling <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Due to the fact that GitHub Release assets cannot be overwritten
atomically, there is an off-chance that the asset we're looking for is
not actually there.

In those rare circumstances when we try to download such an asset while
it has been deleted so that a new version can be uploaded, just wait a
little and try again.

Signed-off-by: Johannes Schindelin <[email protected]>
As downloading the minimal SDK is now so blazing fast, we can avoid
caching it. This saves half a minute that we would otherwise spend on
caching the artifact alone.

Signed-off-by: Dennis Ameling <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@dscho dscho force-pushed the minimal-flavor-from-ci-artifacts branch from 5a3cf09 to 8bfbac8 Compare December 17, 2024 16:52
@dscho dscho changed the title getViaGit: get minimal flavor from ci-artifacts Get the minimal flavor from the ci-artifacts GitHub Release Dec 17, 2024
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

@dennisameling thank you so much for working on this! Sorry it took me so long.

I wanted to ensure a couple of things, and have implemented those now:

  • When the ci-artifacts release is in the process of being updated, I want the Action to play nice and back off, then try again.
  • I want the downloaded archive to be extracted on the fly (why wait until the archive is downloaded entirely?)
  • I wanted the code that accesses the ci-artifacts GitHub Release to be separate from the code that uses git clone, i.e. it should have different home than git.ts, and now it has.
  • I want the change to skip caching the minimal artifact to be separate from the rest so that it can be easily reverted.

It is still blazing fast, and there seems no need whatsoever to "un-skip caching". Exciting!

@dscho
Copy link
Member

dscho commented Dec 17, 2024

Range-diff relative to pre-force-push
  • 1: 5242b29 ! 1: 6db6522 getViaGit: get minimal flavor from ci-artifacts

    @@ Metadata
     Author: Dennis Ameling <[email protected]>
     
      ## Commit message ##
    -    getViaGit: get minimal flavor from ci-artifacts
    +    minimal: get from ci-artifacts
     
    -    As a result of recent changes, the Git SDK `ci-artifacts` are published as GitHub release assets now. This eliminates the need for us to clone and cache the artifacts in this GitHub action.
    +    As a result of recent changes, the Git SDK `ci-artifacts` are published
    +    as GitHub release assets now, including several variants of the minimal
    +    flavor of Git for Windows' SDK. This eliminates the need for us to clone
    +    the minimal SDK in this GitHub Action.
     
    -    Let's simply download the latest `.tar.gz` from the `ci-artifacts` and extract it.
    +    Let's simply download the latest `.tar.gz` from the `ci-artifacts` and
    +    extract it.
    +
    +    Note: As per the analysis in
    +    https://github.com/git-for-windows/git-sdk-64/commit/fdb0cea37389, we
    +    use the native `tar.exe` to unpack the minimal SDK while it is
    +    downloaded, for maximal speed.
    +
    +    The analysis also suggests to use the `.zip` file, as this results in
    +    the fastest operation when download speeds are above 6MB/second (which
    +    we hope will be reliably the case in GitHub Actions). However, we want
    +    to pipe the archive to `tar -xf -` while fetching, and it seems that for
    +    some reason `C:\Windows\system32\tar.exe` misses `.sparse/` and `etc/`
    +    when extracting `.zip` files from `stdin`, but the same is not true with
    +    `.tar.gz` files. So let's use the latter.
    +
    +    See also: https://github.com/git-for-windows/git-sdk-64/pull/87.
     
    -    Ref: https://github.com/git-for-windows/git-sdk-64/commit/fdb0cea373893ce7d40bcfcfbeb7fd091a3c4020
    -    Ref: https://github.com/git-for-windows/git-sdk-64/pull/87
         Signed-off-by: Dennis Ameling <[email protected]>
    +    Signed-off-by: Johannes Schindelin <[email protected]>
     
      ## main.ts ##
    -@@ main.ts: async function run(): Promise<void> {
    -         useCache = true
    -         break
    -       case 'auto':
    --        useCache = flavor !== 'full'
    -+        useCache = !['full', 'minimal'].includes(flavor)
    -         break
    -       default:
    -         useCache = false
    -
    - ## src/git.ts ##
    -@@ src/git.ts: import {ChildProcess, spawn} from 'child_process'
    - import {Octokit} from '@octokit/rest'
    - import {delimiter} from 'path'
    +@@ main.ts: import {
    +   getViaGit,
    +   gitForWindowsUsrBinPath
    + } from './src/git'
    ++import {getViaCIArtifacts} from './src/ci_artifacts'
      import * as fs from 'fs'
    -+import os from 'os'
    -+import fetch, {RequestInit} from 'node-fetch'
      
    - // If present, do prefer the build agent's copy of Git
    - const externalsGitDir = `${process.env.AGENT_HOMEDIRECTORY}/externals/git`
    -@@ src/git.ts: async function updateHEAD(
    -   })
    - }
    + const flavor = core.getInput('flavor')
    +@@ main.ts: async function run(): Promise<void> {
    +     const verbose = core.getInput('verbose')
    +     const msysMode = core.getInput('msys') === 'true'
      
    --export async function getViaGit(
    --  flavor: string,
    --  architecture: string,
    --  githubToken?: string
    --): Promise<{
    -+type GetViaGitResult = {
    -   artifactName: string
    -   id: string
    -   download: (
    -     outputDirectory: string,
    -     verbose?: number | boolean
    -   ) => Promise<void>
    --}> {
    -+}
    +-    const {artifactName, download, id} = await getViaGit(
    +-      flavor,
    +-      architecture,
    +-      githubToken
    +-    )
    ++    const {artifactName, download, id} =
    ++      flavor === 'minimal'
    ++        ? await getViaCIArtifacts(architecture, githubToken)
    ++        : await getViaGit(flavor, architecture, githubToken)
    +     const outputDirectory =
    +       core.getInput('path') || `${getDriveLetterPrefix()}${artifactName}`
    +     core.setOutput('result', outputDirectory)
    +
    + ## src/ci_artifacts.ts (new) ##
    +@@
    ++import * as core from '@actions/core'
    ++import {Octokit} from '@octokit/rest'
    ++import {getArtifactMetadata} from './git'
    ++import {spawn} from 'child_process'
    ++import * as fs from 'fs'
     +
    -+export async function getViaGit(
    -+  flavor: string,
    ++export async function getViaCIArtifacts(
     +  architecture: string,
     +  githubToken?: string
    -+): Promise<GetViaGitResult> {
    -   const owner = 'git-for-windows'
    - 
    -   const {repo, artifactName} = getArtifactMetadata(flavor, architecture)
    - 
    -   const octokit = githubToken ? new Octokit({auth: githubToken}) : new Octokit()
    --  let head_sha: string
    ++): Promise<{
    ++  artifactName: string
    ++  id: string
    ++  download: (
    ++    outputDirectory: string,
    ++    verbose?: number | boolean
    ++  ) => Promise<void>
    ++}> {
    ++  const owner = 'git-for-windows'
     +
    -   if (flavor === 'minimal') {
    --    const info = await octokit.actions.listWorkflowRuns({
    --      owner,
    --      repo,
    --      workflow_id: 938271,
    --      status: 'success',
    --      branch: 'main',
    --      event: 'push',
    --      per_page: 1
    --    })
    --    head_sha = info.data.workflow_runs[0].head_sha
    --    /*
    --     * There was a GCC upgrade to v14.1 that broke the build with `DEVELOPER=1`,
    --     * and `ci-artifacts` was not updated to test-build with `DEVELOPER=1` (this
    --     * was fixed in https://github.com/git-for-windows/git-sdk-64/pull/83).
    --     *
    --     * Work around that by forcing the incorrectly-passing revision back to the
    --     * last one before that GCC upgrade.
    --     */
    --    if (head_sha === '5f6ba092f690c0bbf84c7201be97db59cdaeb891') {
    --      head_sha = 'e37e3f44c1934f0f263dabbf4ed50a3cfb6eaf71'
    --    }
    --  } else {
    --    const info = await octokit.repos.getBranch({
    --      owner,
    --      repo,
    --      branch: 'main'
    --    })
    --    head_sha = info.data.commit.sha
    -+    return getMinimalFlavor(owner, repo, artifactName, octokit, githubToken)
    -   }
    ++  const {repo, artifactName} = getArtifactMetadata('minimal', architecture)
     +
    -+  const info = await octokit.repos.getBranch({
    -+    owner,
    -+    repo,
    -+    branch: 'main'
    -+  })
    -+  const head_sha = info.data.commit.sha
    -   const id = `${artifactName}-${head_sha}${head_sha === 'e37e3f44c1934f0f263dabbf4ed50a3cfb6eaf71' ? '-2' : ''}`
    -   core.info(`Got commit ${head_sha} for ${repo}`)
    - 
    -@@ src/git.ts: export async function getViaGit(
    -     }
    -   }
    - }
    ++  const octokit = githubToken ? new Octokit({auth: githubToken}) : new Octokit()
     +
    -+async function getMinimalFlavor(
    -+  owner: string,
    -+  repo: string,
    -+  artifactName: string,
    -+  octokit: Octokit,
    -+  githubToken?: string
    -+): Promise<GetViaGitResult> {
     +  const ciArtifactsResponse = await octokit.repos.getReleaseByTag({
     +    owner,
     +    repo,
    @@ src/git.ts: export async function getViaGit(
     +    )
     +  }
     +
    ++  core.info(`Found ci-artifacts release: ${ciArtifactsResponse.data.html_url}`)
     +  const tarGzArtifact = ciArtifactsResponse.data.assets.find(asset =>
     +    asset.name.endsWith('.tar.gz')
     +  )
     +
     +  if (!tarGzArtifact) {
     +    throw new Error(
    -+      `Failed to find a tar.gz artifact in the ci-artifacts release of the ${owner}/${repo} repo`
    ++      `Failed to find a .tar.gz artifact in the ci-artifacts release of the ${owner}/${repo} repo`
     +    )
     +  }
     +
    ++  const url = tarGzArtifact.browser_download_url
    ++  core.info(`Found ${tarGzArtifact.name} at ${url}`)
    ++
     +  return {
     +    artifactName,
     +    id: `ci-artifacts-${tarGzArtifact.updated_at}`,
    @@ src/git.ts: export async function getViaGit(
     +      outputDirectory: string,
     +      verbose: number | boolean = false
     +    ): Promise<void> => {
    -+      const tmpFile = `${os.tmpdir()}/${tarGzArtifact.name}`
    -+      core.info(
    -+        `Downloading ${tarGzArtifact.browser_download_url} to ${tmpFile}...`
    -+      )
    -+      await downloadFile(
    -+        tarGzArtifact.browser_download_url,
    -+        {
    -+          headers: {
    -+            ...(githubToken ? {Authorization: `Bearer ${githubToken}`} : {}),
    -+            Accept: 'application/octet-stream'
    -+          }
    -+        },
    -+        tmpFile
    -+      )
    -+      core.info(`Extracting ${tmpFile} to ${outputDirectory}...`)
    -+      fs.mkdirSync(outputDirectory)
    -+      const child = spawn(
    -+        'C:\\Windows\\system32\\tar.exe',
    -+        [`-xz${verbose ? 'v' : ''}f`, tmpFile, '-C', outputDirectory],
    -+        {
    -+          stdio: [undefined, 'inherit', 'inherit']
    -+        }
    -+      )
     +      return new Promise<void>((resolve, reject) => {
    -+        child.on('close', code => {
    -+          if (code === 0) {
    -+            core.info('Finished extracting archive.')
    -+            fs.rm(tmpFile, () => resolve())
    -+          } else {
    -+            reject(new Error(`tar -xzf process exited with code ${code}`))
    ++        const curl = spawn(
    ++          `${process.env.SYSTEMROOT}/system32/curl.exe`,
    ++          [
    ++            ...(githubToken
    ++              ? ['-H', `Authorization: Bearer ${githubToken}`]
    ++              : []),
    ++            '-H',
    ++            'Accept: application/octet-stream',
    ++            `-${verbose === true ? '' : 's'}fL`,
    ++            url
    ++          ],
    ++          {
    ++            stdio: ['ignore', 'pipe', process.stderr]
     +          }
    ++        )
    ++        curl.on('error', error => reject(error))
    ++
    ++        fs.mkdirSync(outputDirectory, {recursive: true})
    ++
    ++        const tar = spawn(
    ++          `${process.env.SYSTEMROOT}/system32/tar.exe`,
    ++          ['-C', outputDirectory, `-x${verbose === true ? 'v' : ''}f`, '-'],
    ++          {stdio: ['pipe', process.stdout, process.stderr]}
    ++        )
    ++        tar.on('error', error => reject(error))
    ++        tar.on('close', code => {
    ++          if (code === 0) resolve()
    ++          else reject(new Error(`tar exited with code ${code}`))
     +        })
    ++
    ++        curl.stdout.pipe(tar.stdin)
     +      })
     +    }
     +  }
     +}
    -+
    -+async function downloadFile(
    -+  url: string,
    -+  options: RequestInit,
    -+  destination: string
    -+): Promise<void> {
    -+  const response = await fetch(url, options)
    -+
    -+  if (!response.ok) {
    -+    throw new Error(`Failed to fetch ${url}: ${response.statusText}`)
    -+  }
    -+
    -+  const fileStream = fs.createWriteStream(destination)
    -+  response.body.pipe(fileStream)
    -+
    -+  return new Promise((resolve, reject) => {
    -+    fileStream.on('finish', resolve)
    -+    fileStream.on('error', reject)
    -+  })
    -+}
  • -: ------- > 2: 3d4ea07 getViaCIArtifacts: add some resilience

  • -: ------- > 3: 64d3606 minimal: avoid caching it

  • 2: 5a3cf09 = 4: 8bfbac8 npm run build && npm run package

Not sure how useful this is ;-) I guess it shows that I almost rewrote the patch... 😊

@dscho dscho merged commit 2ff7f6c into main Dec 17, 2024
8 checks passed
@dscho dscho deleted the minimal-flavor-from-ci-artifacts branch December 17, 2024 18:01
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.

2 participants