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

GHA: use vcpkg to install packages for MSVC jobs #13979

Closed
wants to merge 5 commits into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Jun 21, 2024

You already have vcpkg in your msvc jobs in your ci. You didn't aware of it.
I refactor the vcpkg and add more libraries that can compile with curl.
I added cache that can be restore, and not building every library from scratch every PR, it will use the cache. (save ~7min on openssl and schannel build).

@github-actions github-actions bot added Windows Windows-specific CI Continuous Integration labels Jun 21, 2024
@talregev talregev force-pushed the TalR/vcpkg_windows_ci branch 5 times, most recently from 1d67ad3 to 75bc7ce Compare June 21, 2024 08:18
@talregev
Copy link
Contributor Author

I am closing this PR because I want to debug the cache, and I don't want to always activate curl ci. I will do it on my own ci.

@talregev talregev closed this Jun 21, 2024
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
.github/workflows/windows.yml Outdated Show resolved Hide resolved
@vszakats vszakats mentioned this pull request Jun 21, 2024
@talregev talregev reopened this Jun 21, 2024
@talregev talregev force-pushed the TalR/vcpkg_windows_ci branch 3 times, most recently from a521870 to 8a0017d Compare June 21, 2024 09:07
@talregev
Copy link
Contributor Author

talregev commented Jun 21, 2024

on this line I must use double quotes. The ci will fail on single quotes
https://github.com/curl/curl/pull/13979/files#diff-5ce5c9ff86f58b1f87b35b5227b2e84cb69f022d6741e1854f3e1e181091773cR468

@talregev talregev force-pushed the TalR/vcpkg_windows_ci branch 3 times, most recently from acbd46e to 8817151 Compare June 21, 2024 09:20
@talregev

This comment was marked as resolved.

@vszakats vszakats changed the title msvc vcpkg ci GHA: use vcpkg to install packages for msvc Jun 21, 2024
@vszakats vszakats changed the title GHA: use vcpkg to install packages for msvc GHA: use vcpkg to install packages for MSVC jobs Jun 21, 2024
@vszakats
Copy link
Member

Aside question: Few years ago I remember vcpkg was installing packages from source. Is this still true? On a cursory look the install command finished quickly that hinted this might no longer be the case. If so, do we need caching?

On environment variables I must use double quotes. when I use single quotes it leave the $ symbol, and not give the value of the variable:

export 'tal=$tal'
echo $tal

output: $tal

That's correct. (I wasn't suggesting single quotes in bash string with variables in them)

On the other hand ${{ }} is replaced by GHA before execution, so those can be single-quoted to exclude the chance of an unwanted evaluation.

@talregev
Copy link
Contributor Author

Aside question: Few years ago I remember vcpkg was installing packages from source. Is this still true? On a cursory look the install command finished quickly that hinted this might no longer be the case. If so, do we need caching?

On environment variables I must use double quotes. when I use single quotes it leave the $ symbol, and not give the value of the variable:

export 'tal=$tal'
echo $tal

output: $tal

That's correct. (I wasn't suggesting single quotes in bash string with variables in them)

On the other hand ${{ }} is replaced by GHA before execution, so those can be single-quoted to exclude the chance of an unwanted evaluation.

vcpkg still installing packages from source
Because of my cache implementation on the ci, it use the cache and install the binaries faster.
We need the cache for save ci time.

.github/workflows/windows.yml Outdated Show resolved Hide resolved
@talregev talregev requested a review from vszakats June 27, 2024 18:00
@talregev
Copy link
Contributor Author

@vszakats Can you approve the PR?

@talregev
Copy link
Contributor Author

@vszakats Thank you!

@talregev talregev requested a review from bagder June 30, 2024 16:08
@talregev
Copy link
Contributor Author

@bagder Can you take a look on this PR?

Copy link
Member

@bagder bagder left a comment

Choose a reason for hiding this comment

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

Sorry, I cannot add anything here. Windows and cmake are two weak points of mine. @vszakats is an appropriate authority here!

@vszakats vszakats closed this in e26cbe2 Jun 30, 2024
@vszakats
Copy link
Member

Thank you @talregev, merged now!

@vszakats vszakats added the cmake label Jun 30, 2024
@talregev talregev deleted the TalR/vcpkg_windows_ci branch June 30, 2024 21:17
vszakats added a commit that referenced this pull request Jul 7, 2024
- cache on a per-package basis.
  Replace manual caching with a built-in solution. It shares cached
  package builds between jobs, e.g. libssh2 only builds once
  per platform (instead of once per job). Individual packages are built
  as needed (not the whole per-job tree). It also fixes the duplicate
  cache entry issues.
  Ref: https://learn.microsoft.com/en-us/vcpkg/consume/binary-caching-github-actions-cache
  Follow-up to e26cbe2 #13979
  Follow-up to cb22cfc #14077

- add BoringSSL job with ECH enabled. The first such job in the curl CI.

- add LibreSSL job.

- use vcpkg pre-installed on the runner image, instead of rolling our
  own. This is quicker, simpler and more robust.
  Follow-up to e26cbe2 #13979

- show pre-installed vcpkg and ports version.

- drop `gsasl` dependency till it reaches the pre-installed vcpkg ports.

- re-add `find .` to see the binaries generated.

- simplify setting up `PATH`.

- exclude failing tests for any job enabling WinIDN.

- drop collecting and uploading log archives. We already dump CMake
  logs, and our build doesn't use Ninja. Rest of files weren't generated
  by the curl build. We don't aim to debug vcpkg package builds.

Closes #14090
vszakats added a commit that referenced this pull request Jul 8, 2024
- move `curl --version` into separate step.

- move configure log to separate step. Run on success, too.

- add step with `curl_config.h` dump (full and brief/sorted).

- make `autoreconf` a separate step.

- add each job configuration a short name.

- shorten job names.
  Dedupe/drop redundant info, introduce abbreviations:
  AM = autotools, CM = CMake, U = Unicode, R = Release, not -> `!`, etc.
  Instead of mentioning `debug`, mentioned when it's not.

- simplify `PATH` forming for MSVC jobs.
  It's sufficient to add the release binary directory of vcpkg, the debug one
  is redundant.
  Follow-up to e26cbe2 #13979

- other minor tidy-ups.

Closes #14116
vszakats added a commit to vszakats/curl that referenced this pull request Jul 12, 2024
Follow-up to f99c08d curl#14090
Follow-up to e26cbe2 curl#13979

Closes #xxxxx
vszakats added a commit that referenced this pull request Jul 12, 2024
Now that the package reached the CI runner image.

Follow-up to f99c08d #14090
Follow-up to e26cbe2 #13979

Closes #14170
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration cmake Windows Windows-specific
Development

Successfully merging this pull request may close these issues.

None yet

3 participants