Skip to content

Update ulimit#12576

Merged
dsplaisted merged 4 commits into
dotnet:release/6.0.1xxfrom
dsplaisted:update-ulimit
Nov 3, 2021
Merged

Update ulimit#12576
dsplaisted merged 4 commits into
dotnet:release/6.0.1xxfrom
dsplaisted:update-ulimit

Conversation

@dsplaisted

Copy link
Copy Markdown
Member

@dsplaisted dsplaisted requested review from a team, MattGal and jonfortescue November 1, 2021 21:29
Comment thread build.sh
dockerbuild $args
else
$DIR/run-build.sh $args
sudo $DIR/run-build.sh $args

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Probably merits a similar comment to the other file so folks know why you did it, possibly a link to the tracking issue so it can be reverted if/when this gets addressed by 1ES

@dsplaisted dsplaisted merged commit 2e335b8 into dotnet:release/6.0.1xx Nov 3, 2021
@omajid

omajid commented Nov 8, 2021

Copy link
Copy Markdown
Member

Is defaulting to sudo a good idea? That means everyone trying to build installer needs to have root on their machine. Non root users can't even build it.. Worse, now running as root, the build downloads a whole bunch of files and binaries from the network. Then the build executes all those things as root.

This also makes the source-build dev experience much worse. The build starts off (because I provide my sudo password), then the sudo timeout hits, and the build hangs building at some point asking me to enter my sudo password again.

Maybe this is a workaround for CI, perhaps it should probe for the CI environment and only then do it?

@MattGal

MattGal commented Nov 8, 2021

Copy link
Copy Markdown
Member

Maybe this is a workaround for CI, perhaps it should probe for the CI environment and only then do it?

A fair point but also it's worth reminding ourselves that users with default ulimit -n values are going to hit the same thing just trying to test locally, so it may be worth trying to make the tests fit within a typical limit too

@mmitche

mmitche commented Nov 11, 2021

Copy link
Copy Markdown
Member

This also broke all the official builds as we lost the ambient AzDO environment variables in the build scripts. I think we should revert.

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