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

Update dotnet path in mac publish script #401

Merged

Conversation

keyuxuan
Copy link
Contributor

@keyuxuan keyuxuan commented Aug 23, 2024

The command ./bin/mac/publish would throw error when i run it at the top level of the folder, for example /Users/keyuxuan/microsoft-authentication-cli. The reason is that its using the globally installed dotnet instead of the dotnet under bin/mac folder.

Updating the dotnet to use the absolute path in mac publish script

@keyuxuan keyuxuan marked this pull request as ready for review August 23, 2024 21:12
@keyuxuan keyuxuan requested a review from a team as a code owner August 23, 2024 21:12
@rewrlution
Copy link

Out of curiosity, do we always install dotnet in the bin/mac folder in the build machines?

@keyuxuan
Copy link
Contributor Author

keyuxuan commented Aug 26, 2024

Out of curiosity, do we always install dotnet in the bin/mac folder in the build machines?

good question! based on my understanding, and please correct me if i am wrong:

i dont think we are really installing dotnet in the bin/mac folder here, the purpose of dotnet script is that it tries to populate an environment variable ADO_TOKEN with an access token using azureauth. And This ADO_TOKEN is being used while doing dotnet publishing.

So before this PR's change, in the publish script, its calling dotnet publish directly without that obtaining the token step, thats why there were some errors (which seem to be authentication errors/issues)

@mvanchaa
Copy link
Contributor

Out of curiosity, do we always install dotnet in the bin/mac folder in the build machines?

good question! based on my understanding, and please correct me if i am wrong:

i dont think we are really installing dotnet in the bin/mac folder here, the purpose of dotnet script is that it tries to populate an environment variable ADO_TOKEN with an access token using azureauth. And This ADO_TOKEN is being used while doing dotnet publishing.

So before this PR's change, in the publish script, its calling dotnet publish directly without that obtaining the token step, thats why there were some errors (which seem to be authentication errors/issues)

To add to what Keyu mentioned, we don't install dotnet in any of the publish or dotnet scripts under the bin/mac folder. In the dotnet script, after warming the ADO_TOKEN, we invoke the globally installed dotnet.

@rewrlution
Copy link

Out of curiosity, do we always install dotnet in the bin/mac folder in the build machines?

good question! based on my understanding, and please correct me if i am wrong:
i dont think we are really installing dotnet in the bin/mac folder here, the purpose of dotnet script is that it tries to populate an environment variable ADO_TOKEN with an access token using azureauth. And This ADO_TOKEN is being used while doing dotnet publishing.
So before this PR's change, in the publish script, its calling dotnet publish directly without that obtaining the token step, thats why there were some errors (which seem to be authentication errors/issues)

To add to what Keyu mentioned, we don't install dotnet in any of the publish or dotnet scripts under the bin/mac folder. In the dotnet script, after warming the ADO_TOKEN, we invoke the globally installed dotnet.

I see. that makes sense! approved!

@rewrlution rewrlution closed this Aug 26, 2024
@rewrlution rewrlution reopened this Aug 26, 2024
Copy link

@rewrlution rewrlution left a comment

Choose a reason for hiding this comment

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

Ship it!

@keyuxuan keyuxuan merged commit 35dbe94 into main Aug 26, 2024
13 checks passed
@keyuxuan keyuxuan deleted the user/keyuxuan/update-dotnet-path-in-mac-publish-script branch August 26, 2024 18:52
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.

4 participants