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

Wrap the dotnet executable to set default environment variables #102

Merged
merged 21 commits into from
Dec 3, 2019
Merged

Wrap the dotnet executable to set default environment variables #102

merged 21 commits into from
Dec 3, 2019

Conversation

jrbeverly
Copy link
Contributor

@jrbeverly jrbeverly commented Nov 21, 2019

While working on #98 I encountered difficulty while trying to run the dotnet.exe cross-platform. It has the requirement that the environment variables HOME, DOTNET_CLI_HOME, APPDATA, PROGRAMFILES are defined.

Although constants could works (conditional by OS), this means that properties of a bazel dir could impact how a build compiles. Instead, this PR creates a wrapper dotnetw that sets environment variables before execution.

This is a draft at the moment while I iron about some edge cases (and refactor the wrapper)

Related to #10

@jrbeverly jrbeverly added the bug Something isn't working label Nov 21, 2019
@jrbeverly jrbeverly self-assigned this Nov 21, 2019
@jrbeverly jrbeverly marked this pull request as ready for review November 25, 2019 15:12
@jrbeverly jrbeverly requested a review from j3parker November 25, 2019 15:13
@j3parker j3parker removed the bug Something isn't working label Nov 27, 2019
Copy link
Member

@j3parker j3parker left a comment

Choose a reason for hiding this comment

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

Looks great! Just some minor stuff/questions.

@jrbeverly jrbeverly requested a review from j3parker November 27, 2019 20:05
@apriyadarsheeD2L
Copy link

Were you able to do your testing on Mac?

@jrbeverly
Copy link
Contributor Author

Yes, for commit 8d0ef05 I changed to list initialization and enforced const_char on the string values. That resolved the issue I was seeing on MacOS.

Seems that the MacOS version of execve has strict expectations with the inputs being const.

default = Label(_TEMPLATE),
allow_single_file = True
),
"src": attr.label_list(
Copy link
Member

Choose a reason for hiding this comment

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

nit: this file isn't really used at compile time so it's a tiny bit weird to call it src, but that's fine. This is an internal rule.


/*
dotnet requires these environment variables to be set.
*/
Copy link
Member

Choose a reason for hiding this comment

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

nit: inconsistent comment style in this file

@jrbeverly jrbeverly merged commit 15ed431 into Brightspace:master Dec 3, 2019
@jrbeverly jrbeverly deleted the dotnet/wrap-the-dotnet-exe branch December 3, 2019 14:28
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