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

build: document env vars expected to be set for the CI configs #15129

Merged
merged 2 commits into from Oct 19, 2018

Conversation

Projects
None yet
5 participants
@alexeykuzmin
Contributor

alexeykuzmin commented Oct 12, 2018

Description of Change

Document expected variables to be provided externally on each CI platform.
/cc @jkleinsc, @nornagon

Checklist
  • PR description included and stakeholders cc'd
  • npm test passes
  • PR title follows semantic commit guidelines
Release Notes

Notes: no notes

@alexeykuzmin alexeykuzmin requested review from electron/reviewers as code owners Oct 12, 2018

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 13, 2018

This is going to break everyone's build right? Is their a technical reason for this change apart from it just being a better name?

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 13, 2018

Is their a technical reason for this change apart from it just being a better name?

@MarshallOfSound
The goal is to check if we can do it, and I'm more concerned about CI builds than local.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 13, 2018

@jkleinsc Looks like for VSTS and AppVeyor we store some env vars in a server configuration.
Can we move them to corresponding configs to be able to use different values in different branches, and keep track of the changes?

@jkleinsc

This comment has been minimized.

Contributor

jkleinsc commented Oct 15, 2018

@alexeykuzmin the problem is that for both VSTS and Appveyor we don't have different jobs defined in the config, but rather we use the environment variables to drive which type of job is run (debug, testing, or release).

For Appveyor I suggest we leave the environment variable (GN_CONFIG) in place and then add an if statement around this line:
https://github.com/electron/electron/blob/master/appveyor.yml#L32
eg.

  - ps: >-
      if ($env:GN_CONFIG -eq 'release') {
        gn gen out/Default "--args=import(\"//electron/build/args/electron_release.gn\") $env.GN_EXTRA_ARGS"
      } elseif  ($env:GN_CONFIG -eq 'testing') {
         gn gen out/Default "--args=import(\"//electron/build/args/electron_testing.gn\") $env.GN_EXTRA_ARGS"
      } else {
        gn gen out/Default "--args=import(\"//electron/build/args/electron_debug.gn\") $env.GN_EXTRA_ARGS"
      }

For VSTS we could use the if [ "$ELECTRON_RELEASE" == "1" ]; then conditional, but that wouldn't account for future use of debug builds.

@nornagon

i don't think those are better names

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 16, 2018

@nornagon

don't think those are better names

The goal is to check if we can do it, and I'm more concerned about CI builds than local.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 16, 2018

@jkleinsc can you please review the env variables lists and descriptions I added to the CI configs?

@jkleinsc

Overall LGTM, but as @nornagon mentioned, I'm not sure why we need to change the arg file names.

Show resolved Hide resolved .circleci/config.yml
# The config expects the following environment variables to be set:
# - "GN_CONFIG" Build type. One of {'debug', 'testing', 'release'}.
# - "GN_EXTRA_ARGS" Additional gn arguments for a build config,
# e.g. 'fatal_linker_warnings=false enable_linux_installer=false'.

This comment has been minimized.

@jkleinsc

jkleinsc Oct 16, 2018

Contributor

Given that appveyor is Windows only maybe use Windows example, eg target_cpu = \"x86\" for 32 bit builds

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 17, 2018

Contributor

Done

# e.g. 'fatal_linker_warnings=false enable_linux_installer=false'.
# Use it to set 'target_cpu' for example.
# - "ELECTRON_RELEASE" Define it to upload binaries on success.
# - "NPM_CONFIG_ARCH" E.g. 'ia32'. Is used to build native Node.js modules.

This comment has been minimized.

@jkleinsc

jkleinsc Oct 16, 2018

Contributor

This could also be windows specific, eg x86.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 17, 2018

Contributor

Done

vsts.yml Outdated
# - "GN_EXTRA_ARGS" Additional gn args, e.g. 'is_mas_build=true'.
# - "NOTIFY_SLACK" Set it to '1' to enable Slack notifications.
# - "RUN_TESTS" Set it to '1' to run Electron's tests.
# - "SLACK_WEBHOOK" Slack hook URL to send notifications.

This comment has been minimized.

@jkleinsc

jkleinsc Oct 16, 2018

Contributor

This variable is slack_webhook and is a private variable meaning you can't access it as an environment variable, so I'm not sure we need to document here.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 17, 2018

Contributor

Removed

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 17, 2018

I'm not sure why we need to change the arg file names.

@jkleinsc I'm going to revert the renames. It was never a goal anyway.
cc @nornagon

UPD Done

@alexeykuzmin alexeykuzmin changed the title from build: add "electron_" prefix to the Electron build configs to [wip] build: document env vars expected to be set for the CI configs Oct 17, 2018

@alexeykuzmin alexeykuzmin changed the title from [wip] build: document env vars expected to be set for the CI configs to build: document env vars expected to be set for the CI configs Oct 17, 2018

@@ -29,7 +50,8 @@ build_script:
"https://github.com/electron/electron"
- gclient sync --with_branch_heads --with_tags
- cd src
- gn gen out/Default "--args=import(\"//electron/build/args/%GN_CONFIG%.gn\") %GN_EXTRA_ARGS%"
- ps: $env:BUILD_CONFIG_PATH="//electron/build/args/%GN_CONFIG%.gn"

This comment has been minimized.

@jkleinsc

jkleinsc Oct 17, 2018

Contributor

This change isn't really needed. I don't think that using % for environment variables in powershell works.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 17, 2018

Contributor

I agree that it's not absolutely needed. But in the future we will get a cleaner diff when we decide to change that path.

I don't think that using % for environment variables in powershell works.

All Windows builds are green =)

This comment has been minimized.

@jkleinsc

jkleinsc Oct 17, 2018

Contributor

@alexeykuzmin yes, I saw the builds are green, but when I try that in powershell, I see the following:

PS C:\Users\jkleinsc> $env:GN_CONFIG="testing"

PS C:\Users\jkleinsc> $env:BUILD_CONFIG_PATH="//electron/build/args/%GN_CONFIG%.gn"

PS C:\Users\jkleinsc>echo $env:BUILD_CONFIG_PATH
//electron/build/args/%GN_CONFIG%.gn

This comment has been minimized.

@jkleinsc

jkleinsc Oct 17, 2018

Contributor

maybe appveyor is smart enough to convert or something.

This comment has been minimized.

@nornagon

nornagon Oct 19, 2018

Contributor

i don't understand how this works, why would the %% be substituted in powershell?

# - "GN_EXTRA_ARGS" Additional gn arguments for a build config,
# e.g. 'target_cpu="x86"' to build for a 32bit platform.
# https://gn.googlesource.com/gn/+/master/docs/reference.md#target_cpu
# - "ELECTRON_RELEASE" Define it to upload binaries on success.

This comment has been minimized.

@nornagon

nornagon Oct 17, 2018

Contributor

be specific about define it. should it be set to 1 or "true" or "yes"?

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 18, 2018

Contributor

Any value that would cause powershell's Test-Path Env:\ELECTRON_RELEASE to be evaluated to True works.

This comment has been minimized.

@nornagon

nornagon Oct 18, 2018

Contributor

Cool, pick one and document it here please.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 18, 2018

Contributor

@jkleinsc do we usually set ELECTRON_RELEASE to 1?

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 19, 2018

Member

@alexeykuzmin Yes we normally set ELECTRON_RELEASE to 1 and UPLOAD_TO_S3 to 1 when we need them defined.

In some cases we explicitly check for the 1 value: https://github.com/electron/electron/blob/master/.circleci/config.yml#L218

Show resolved Hide resolved appveyor.yml
# Is used in some publishing scripts, but does NOT affect the Electron binary.
# - "UPLOAD_TO_S3" Define it to upload a release to the S3 bucket.
# If it's not defined the release is uploaded to the Github Releases.
# The value is only checked if "ELECTRON_RELEASE" is defined.

This comment has been minimized.

@nornagon

nornagon Oct 17, 2018

Contributor

if ELECTRON_RELEASE causes binaries to be uploaded, what does this do differently?

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 18, 2018

Contributor

ELECTRON_RELEASE means that binaries should be uploaded, UPLOAD_TO_S3 controls where exactly.

This comment has been minimized.

@nornagon

nornagon Oct 18, 2018

Contributor

oh, is UPLOAD_TO_S3 supposed to be a bucket name then? That's totally not clear from the comment.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Oct 18, 2018

Contributor

No, it's not a bucket =) It's just a flag:

Define it to upload a release to the S3 bucket.
If it's not defined the release is uploaded to the Github Releases.

@jkleinsc can probably explain it better.

This comment has been minimized.

@MarshallOfSound

MarshallOfSound Oct 19, 2018

Member

@nornagon ELECTRON_RELEASE tells CI to (a) build for release and (b) upload those release files

By default those uploads go to Github Release, if you define UPLOAD_TO_S3 it will instead upload them to S3

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Oct 19, 2018

@nornagon Should be good now.
cc @jkleinsc if you want to make any comments.

@jkleinsc jkleinsc self-requested a review Oct 19, 2018

@jkleinsc

LGTM

@nornagon

LGTM but that powershell var substitution looks weird, please figure out why that's working

@alexeykuzmin alexeykuzmin merged commit ba7ce72 into master Oct 19, 2018

21 checks passed

Absolute Zero
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-debug AppVeyor build succeeded
Details
appveyor: win-ia32-testing AppVeyor build succeeded
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-debug AppVeyor build succeeded
Details
appveyor: win-x64-testing AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

release-clerk bot commented Oct 19, 2018

No Release Notes

@trop

This comment has been minimized.

Contributor

trop bot commented Oct 19, 2018

We have automatically backported this PR to "4-0-x", please check out #15301

@trop trop bot added merged/4-0-x and removed target/4-0-x labels Oct 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment