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

Remove bash dependency from init-compiler #11359

Merged
merged 4 commits into from
Dec 20, 2022

Conversation

am11
Copy link
Member

@am11 am11 commented Oct 21, 2022

No description provided.

@am11 am11 force-pushed the feature/shell-scripts branch from 14453e9 to 5ad6cc5 Compare October 21, 2022 12:39
@am11 am11 marked this pull request as ready for review October 21, 2022 13:41
compiler=gcc
;;
esac

cxxCompiler="$compiler++"

. "$nativescriptroot"/../pipeline-logging-functions.sh
Copy link
Member

Choose a reason for hiding this comment

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

@dotnet/roslyn-infrastructure This is deleting eng system telemetry. Why was this telemetry added in the first place? Are we ok with deleting it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it was added to satisfy validation in eng/configure-toolset.sh, that has an ignore list which I have updated below.

Note that when we invoke build.sh in runtime and diagnostics repos, which call multiple scripts before reaching init-compiler.sh, we use plain echo or print commands in all those scripts. This script only has a few error cases, which we don't encounter in the CI, so I replaced Write-PipelineTelemetryError with echo.

Copy link
Member

Choose a reason for hiding this comment

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

We do not encounter these error cases in the CI when everything is working correctly. We may encounter these error cases in the CI on infrastructure updates or when things break for some reason. IIRC, it is why this telemetry was added in the first place - to allow eng system team to see that there is a problem in some repo after the update.

Copy link
Member Author

@am11 am11 Oct 26, 2022

Choose a reason for hiding this comment

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

When it was added, this script was only used by arcade cmake SDK. I deduplicated that code in dotnet/runtime#59018 just to avoid duplication and not for telemetry. The one invovled in the build had no stake in telemetry data. I don't think out of all the other possible errors in 20+ scripts involved in the build (which use echo), there is anything special about the few in this script.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree with @am11, the telemetry here is not super valuable, I've never actually seen it reporting a failure.

@am11 would you mind resolving the merge conflicts? I think this is good to go.

@akoeplinger akoeplinger merged commit 79e739d into dotnet:main Dec 20, 2022
@am11 am11 deleted the feature/shell-scripts branch December 20, 2022 14:21
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.

3 participants