Update PHP & Ruby Mac BUILD #24860

veblush · 2020-12-01T02:02:46Z

This is mainly to use Mojave for PHP, Ruby, and BuildArtifacts.

Disable HOMEBREW_UPDATE to prevent unexpected update failure.
Simplify Ruby installation
Simplify Python installation
- Python 3.7 and 3.8 are pre-installed.
- MD5 check is replaced with SHA256 check not to update Homebrew.
Updated PHP version to 7.2 to be aligned with the existing PHPUnit (and 7.2 is the oldest version among the supported ones)
Remove a protoc x86 artifact since there is no x86 build-tool on Mojave.
Use -mmacosx-version-min=10.10 to be consistent with other gRPC artifacts.

tools/internal_ci/helper_scripts/prepare_build_macos_rc

stanley-cheung

Thanks for taking care of this! And just to confirm: PHP 7.2 and PHPUnit 8.x are the correct versions where we want to be at.

jtattermusch

This is generally looking fine, but I have a few suggestions.

Also, please make sure you're not breaking Grpc.Tools nuget package build (currently that's the case).

jtattermusch · 2020-12-01T13:11:50Z

tools/internal_ci/helper_scripts/prepare_build_macos_rc

@@ -25,6 +25,9 @@ ulimit -a
 # - try adding a dependency under a language-specific section first (reduces latency and increases build stability)
 # - only add stuff that you absolutely need for your builds to work (add comment to explain why its needed)

+# Disable HOMEBREW update to avoid new updates which potentially have problems.
+export HOMEBREW_NO_AUTO_UPDATE=1


sg.
suggestion: would it be cleaner to also explictly checkout homebrew-core so that things are at a known state?
Like this we are relying on what is the last commit available on kokoro macos workers and we have no control of that. I think it would be worth checking what's the commit provided by the current mojave kokoro workers and pinning it (or at least mentioning it in a comment, so that troubleshooting is easier).

I briefly updated it to include more info on why its update is disabled and the current version of the core repository. I didn't fix the version because it's meant to use the version when the image was built and it will change when migrating to Catalina or Big sure in future.

jtattermusch · 2020-12-01T13:12:31Z

tools/internal_ci/helper_scripts/prepare_build_macos_rc

  for RUBY_VERSION in 2.5.0 2.7.0; do
-    # TODO(jtattermusch): find a better way of installing ruby, as the current way installs a huge number


sg if all ruby builds still work fine after this.

Yep it works :)

jtattermusch · 2020-12-01T13:13:01Z

tools/internal_ci/helper_scripts/prepare_build_macos_rc

@@ -129,8 +96,15 @@ then
  export DOTNET_CLI_TELEMETRY_OPTOUT=true
 fi

-# PHP tests currently require using an older version of PHPUnit
-ln -sf /usr/local/bin/phpunit-5.7 /usr/local/bin/phpunit
+if [ "${PREPARE_BUILD_INSTALL_DEPS_PHP}" == "true" ]


+1, very nice.

jtattermusch · 2020-12-01T13:15:57Z

tools/run_tests/artifacts/artifact_targets.py

@@ -348,7 +348,6 @@ def targets():
        ProtocArtifact('linux', 'x64'),
        ProtocArtifact('linux', 'x86'),
        ProtocArtifact('macos', 'x64'),
-        ProtocArtifact('macos', 'x86'),


as is, this will break the build of Grpc.Tools nuget package (run an adhoc artifact/packages/distribtest flow to see that)
two options:

we remove the x86 protoc artifact in a separate PR (and I can help with adjusting C#).

you also include the Grpc.Tools fix here

Removing these should be enough to make the nuget package build pass?

grpc/src/csharp/Grpc.Tools/Grpc.Tools.csproj

Line 72 in 2dc98b1

<_Asset PackagePath="tools/linux_x64/" Include="$(Assets_GrpcPlugins)protoc_linux_x64/grpc_csharp_plugin" />

grpc/src/csharp/Grpc.Tools/Grpc.Tools.csproj

Line 65 in 2dc98b1

<_Asset PackagePath="tools/macosx_x86/" Include="$(Assets_ProtoCompiler)macos_x86/protoc" />

Thanks! I included this in this PR.

jtattermusch

LGTM once my comments are resolved and tests are looking good (including the artifact - package build - distribtest flow).

jtattermusch · 2020-12-01T15:47:02Z

Artifact macos build fails with:

+ python3.8 -m pip install -U cython setuptools==44.1.1 wheel --user
github/grpc/tools/internal_ci/macos/grpc_build_artifacts.sh: line 32: python3.8: command not found

https://source.cloud.google.com/results/invocations/b134b8fd-af31-4133-a552-1c46bf8e7b3f/targets/grpc%2Fcore%2Fpull_request%2Fmacos%2Fgrpc_build_artifacts/log

veblush · 2020-12-01T18:25:46Z

Artifact macos build fails with:
+ python3.8 -m pip install -U cython setuptools==44.1.1 wheel --user
github/grpc/tools/internal_ci/macos/grpc_build_artifacts.sh: line 32: python3.8: command not found
https://source.cloud.google.com/results/invocations/b134b8fd-af31-4133-a552-1c46bf8e7b3f/targets/grpc%2Fcore%2Fpull_request%2Fmacos%2Fgrpc_build_artifacts/log

It is expected because this PR assumes that it runs on Mojave machines while the presubmit is still using High Sierra which doesn't have Python 3.7 and 3.8 installed.

veblush · 2020-12-01T18:26:45Z

FYI, this PR has been tested with the Adhoc MacOS job on Mojave for PHP, Python, Ruby, and BuildArtifacts.

jtattermusch · 2020-12-02T14:39:40Z

@veblush #24870 now hopefully has all the necessary changes to drop protoc x86 macos artifacts without breaking the package build (a ruby package fix was also necessary).

jtattermusch · 2020-12-03T12:51:00Z

#24870 has been merged - you can rebase now.

jtattermusch · 2020-12-03T12:53:43Z

tools/run_tests/artifacts/artifact_targets.py

@@ -95,7 +95,7 @@ def create_jobspec(name,
    return jobspec


-_MACOS_COMPAT_FLAG = '-mmacosx-version-min=10.7'
+_MACOS_COMPAT_FLAG = '-mmacosx-version-min=10.10'


qq: is this really needed?

This doesn't need to be part of this PR but I just make this consistent with the all other settings for OSX.

jtattermusch · 2020-12-03T13:03:48Z

Artifact macos build fails with:
+ python3.8 -m pip install -U cython setuptools==44.1.1 wheel --user
github/grpc/tools/internal_ci/macos/grpc_build_artifacts.sh: line 32: python3.8: command not found
https://source.cloud.google.com/results/invocations/b134b8fd-af31-4133-a552-1c46bf8e7b3f/targets/grpc%2Fcore%2Fpull_request%2Fmacos%2Fgrpc_build_artifacts/log
It is expected because this PR assumes that it runs on Mojave machines while the presubmit is still using High Sierra which doesn't have Python 3.7 and 3.8 installed.

Got it, so you plan to merge this PR at the same time as submitting the cl that upgrades the mac jobs to mojave, right?

That kind of works, but I'd prefer this approach:

add a simple check whether python3.7 and 3.8 is available, and if not, run the existing commands to install them (this is pretty trivial to do) + add a TODO to remove later
that way, the macos artifact build (and all the other jobs that already pass) will pass on both high-sierra and mojave. That's a much better situation to be in since you can then submit your CL independently without breaking stuff and gives you rollback option in case issues arise - for contrast, if you rely on merging PR & CL in a lockstep and things break for some reason (which can easily happen in the complicated testing world we live in), you'll find yourself in a difficult situation that's hard to fix quickly and without impacting others.
once everything is running on macos mojave for a few days and things seem stable, you can remove the conditional python3.7 and python3.8 installation.

veblush · 2020-12-04T00:08:43Z

@jtattermusch You're right about my intention. I planned to do interlocked update with this and the internal CL because it doesn't seem feasible to make this run on both High Sierra and Mojave due to Ruby and PHP. Somehow it seems to work so let me try to make this runnable on both. Having Python 3.7 and 3.8 installed on High Sierra with conditional statements shouldn't be hard.

veblush · 2020-12-04T04:58:55Z

I'll merge this tomorrow (Dec 4) morning so that I can take action when something goes wrong.

jtattermusch · 2020-12-04T09:12:08Z

The ObjC test failures seems like a flake unrelated to this PR: https://source.cloud.google.com/results/invocations/4c06f976-a8a9-4620-bb0e-d79a9a2e152f/targets/github%2Fgrpc%2Frun_tests%2Fobjc_macos_opt_native%2Fios-test-interoptests/tests

jtattermusch · 2020-12-04T09:14:23Z

@veblush This PR is looking good, so I'll actually merge it now to get the advantage of getting a few runs in before (your) morning.

veblush added lang/all wrapped languages platform/macOS area/build platform/iOS release notes: no Indicates if PR should not be in release notes labels Dec 1, 2020

veblush requested review from jtattermusch, stanley-cheung and lidizheng December 1, 2020 02:02

lidizheng approved these changes Dec 1, 2020

View reviewed changes

tools/internal_ci/helper_scripts/prepare_build_macos_rc Show resolved Hide resolved

stanley-cheung approved these changes Dec 1, 2020

View reviewed changes

veblush force-pushed the kokoro-mac-mojave branch from 86414ec to b0a9352 Compare December 1, 2020 07:48

jtattermusch reviewed Dec 1, 2020

View reviewed changes

jtattermusch approved these changes Dec 1, 2020

View reviewed changes

lidizheng added the kokoro:run label Dec 1, 2020

grpc-kokoro removed the kokoro:run label Dec 1, 2020

jtattermusch mentioned this pull request Dec 2, 2020

Stop building protoc x86 artifacts on macos #24870

Merged

jtattermusch reviewed Dec 3, 2020

View reviewed changes

veblush added 4 commits December 3, 2020 15:08

Update PHP & Ruby Mac BUILD

76afc77

Check sha256

7018288

Update prepare_build_macos_rc

ead79e7

Conditional python installation

3a01cfd

veblush force-pushed the kokoro-mac-mojave branch from 4297d44 to 3a01cfd Compare December 3, 2020 23:19

jtattermusch merged commit 17a7df4 into grpc:master Dec 4, 2020

yulin-liang added a commit to yulin-liang/grpc that referenced this pull request Jan 12, 2021

backport of grpc#24860

7a7e67f

yulin-liang mentioned this pull request Jan 12, 2021

[Backport]Fix macos distribtest #25135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update PHP & Ruby Mac BUILD #24860

Update PHP & Ruby Mac BUILD #24860

veblush commented Dec 1, 2020 •

edited

stanley-cheung left a comment •

edited

jtattermusch left a comment

jtattermusch Dec 1, 2020

veblush Dec 1, 2020

jtattermusch Dec 1, 2020

veblush Dec 1, 2020

jtattermusch Dec 1, 2020

jtattermusch Dec 1, 2020

jtattermusch Dec 1, 2020

veblush Dec 1, 2020

jtattermusch left a comment

jtattermusch commented Dec 1, 2020

veblush commented Dec 1, 2020

veblush commented Dec 1, 2020

jtattermusch commented Dec 2, 2020

jtattermusch commented Dec 3, 2020

jtattermusch Dec 3, 2020

veblush Dec 4, 2020

jtattermusch commented Dec 3, 2020

veblush commented Dec 4, 2020 •

edited

veblush commented Dec 4, 2020

jtattermusch commented Dec 4, 2020

jtattermusch commented Dec 4, 2020

		for RUBY_VERSION in 2.5.0 2.7.0; do
		# TODO(jtattermusch): find a better way of installing ruby, as the current way installs a huge number

Update PHP & Ruby Mac BUILD #24860

Update PHP & Ruby Mac BUILD #24860

Conversation

veblush commented Dec 1, 2020 • edited

stanley-cheung left a comment • edited

Choose a reason for hiding this comment

jtattermusch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch left a comment

Choose a reason for hiding this comment

jtattermusch commented Dec 1, 2020

veblush commented Dec 1, 2020

veblush commented Dec 1, 2020

jtattermusch commented Dec 2, 2020

jtattermusch commented Dec 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtattermusch commented Dec 3, 2020

veblush commented Dec 4, 2020 • edited

veblush commented Dec 4, 2020

jtattermusch commented Dec 4, 2020

jtattermusch commented Dec 4, 2020

veblush commented Dec 1, 2020 •

edited

stanley-cheung left a comment •

edited

veblush commented Dec 4, 2020 •

edited