Skip to content

Main#12

Merged
reutermj merged 1 commit intomainfrom
winsdk_license
Jun 29, 2025
Merged

Main#12
reutermj merged 1 commit intomainfrom
winsdk_license

Conversation

@reutermj
Copy link
Copy Markdown
Owner

No description provided.

Copilot AI review requested due to automatic review settings June 29, 2025 17:31
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR centralizes the Windows SDK license acceptance check into the lazy-download step, adds parsing for the license-acceptance env var, and refactors a special-case toolchain naming prefix.

  • Moved and simplified Windows SDK license check from cxx_toolchains into lazy_download_bins
  • Parsed accept_winsdk_license from an env var in get_config_from_env_vars
  • Refactored default toolchain naming logic in toolchains_cc.bzl

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
toolchains_cc.bzl Removed commented-out Windows license block and refactored toolchain_name handling
impl/declare_tools.bzl Added Windows SDK license acceptance check in _lazy_download_bins_impl
impl/config.bzl Introduced accept_winsdk_license parsing from an env var in config
Comments suppressed due to low confidence (1)

impl/config.bzl:45

  • [nitpick] New license-parsing logic should have unit tests verifying both True and False branches when the env var is set or unset.
    accept_winsdk_license_str = rctx.getenv(accept_winsdk_license_var)

vendor = triple.split("-")[1]
libc_version = triple.split("-")[3]

accept_winsdk_license_var = "{}_accept_winsdk_license".format(toolchain_name)
Copy link

Copilot AI Jun 29, 2025

Choose a reason for hiding this comment

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

If toolchain_name is never passed in, it defaults to None, resulting in an env var named None_accept_winsdk_license. Consider defaulting toolchain_name to rctx.attr.toolchain_name or requiring it to be provided so the correct variable is read.

Copilot uses AI. Check for mistakes.
@reutermj reutermj merged commit 410883d into main Jun 29, 2025
4 checks passed
@reutermj reutermj deleted the winsdk_license branch June 29, 2025 17:33
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