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

readonly variable warning on Azure pipelines #68671

Closed
Mark-Simulacrum opened this issue Jan 30, 2020 · 8 comments · Fixed by #69894
Closed

readonly variable warning on Azure pipelines #68671

Mark-Simulacrum opened this issue Jan 30, 2020 · 8 comments · Fixed by #69894
Assignees
Labels
P-high High priority T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

Overwriting readonly task variable 'RUST_CONFIGURE_ARGS'. This behavior will be disabled in the future. See https://github.com/microsoft/azure-pipelines-yaml/blob/master/design/readonly-variables.md for details.

@Mark-Simulacrum Mark-Simulacrum added P-high High priority T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. I-nominated and removed P-high High priority labels Jan 30, 2020
@pietroalbini pietroalbini added P-high High priority and removed I-nominated labels Feb 5, 2020
@aidanhs
Copy link
Member

aidanhs commented Feb 12, 2020

@nellshamrell
Copy link
Contributor

I can take a look at this this week

@pietroalbini
Copy link
Member

We're waiting on a timeline from Microsoft on the removal.

@nellshamrell
Copy link
Contributor

nellshamrell commented Mar 6, 2020

Sent a follow up email to Microsoft today:

Hello there and greetings from Mozilla!

I'd like to follow up on the transition to read only variables (and away from overwriting read only variables) in Azure pipelines. We are now seeing a warning about this in our Azure pipeline output. Do you have any timeline on when the behavior of overwriting readonly variables will be removed? We'd love to have this information so we know how to prioritize our backlog of work.

@nellshamrell
Copy link
Contributor

Hello all - turning my attention to this this week.

I wrote up what I believe the problem is - could someone confirm that this is correct?

RUST_CONFIG_ARGS is initially set in src/ci/azure-pipelines/auto.yml for the various platforms we build on.

Let's first focus on windows x86_64-msvc-2

src/ci/azure-pipeines/auto.yml

- job: Windows
  timeoutInMinutes: 600
  pool:
    vmImage: 'vs2017-win2016'
  steps:
  - template: steps/run.yml
  strategy:
    matrix:
      # 32/64 bit MSVC tests
      (...)
       x86_64-msvc-2:
        RUST_CONFIGURE_ARGS: --build=x86_64-pc-windows-msvc --enable-profiler
        SCRIPT: make ci-subset-2

Now let's look at the template used in src/ci/azure-pipeines/steps/run.yml

There are a number of scripts called - but let's take a closer look at this one:

- bash: src/ci/scripts/install-clang.sh
  displayName: Install clang
  condition: and(succeeded(), not(variables.SKIP_JOB))

It calls the install-clang script - let's take a closer look at it.

elif isWindows && [[ ${CUSTOM_MINGW-0} -ne 1 ]]; then
    (...)
    mkdir -p citools
    cd citools
    curl -f "${MIRRORS_BASE}/LLVM-9.0.0-win64.tar.gz" | tar xzf -
    ciCommandSetEnv RUST_CONFIGURE_ARGS \
        "${RUST_CONFIGURE_ARGS} --set llvm.clang-cl=$(pwd)/clang-rust/bin/clang-cl.exe"
fi

Notice how it is overwriting RUST_CONFIGURE_ARGS? This is the reason we are getting this warning about overwriting read only variables (among many others).

@pietroalbini
Copy link
Member

That sounds correct!

@pietroalbini
Copy link
Member

@nellshamrell a quick fix for this would be to set in the job definition something like INITIAL_RUST_CONFIGURE_ARGS and then set RUST_CONFIGURE_ARGS to it from a script.

@vtbassmatt
Copy link
Contributor

👋 PM from Azure Pipelines here. @nellshamrell and @pietroalbini you have it exactly correct. We intended that matrix variables would become read-only, but I didn't do a good job communicating about that. So sorry that you got caught out by this (and further sorry that we didn't get right people looped in until the change took effect!).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants