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

correct docs: direnv can't save ulimit #17561

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

michaelglass
Copy link
Contributor

direnv can only cache env variables, not ulimit settings

see direnv/direnv#777

direnv can only cache env variables, not ulimit settings
@michaelglass michaelglass changed the title direnv can't save ulimit correct docs: direnv can't save ulimit Nov 17, 2022
@@ -233,8 +233,7 @@ This can be fixed by setting `ulimit -n 10000`. (10,000 should work in all cases
> We recommend permanently setting this by either:
>
> 1. Adding `ulimit -n 10000` to your `./pants` script.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, while being here, I think we might as well update the recommendation to patch the ./pants script to instead refer to the .pants.bootstrap script as a means to patch the pants runtime env, if you don't mind :)

https://github.com/pantsbuild/setup/blob/ab8581bb52d92fcb3fda1b1dfb20204ae8888a90/pants#L19-L23

Perfectly fine if not, I wasn't aware of this patching pants script being a thing in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
> 1. Adding `ulimit -n 10000` to your `./pants` script.
> 1. Adding `ulimit -n 10000` to your `./pants.bootstrap` script.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, minor nit, there's a missing leading . on the file name :)

Copy link
Member

Choose a reason for hiding this comment

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

@Eric-Arellano oops, I missed this convo was marked as resolved..

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is supposed to be ./pants bootstrap script?

Copy link
Member

Choose a reason for hiding this comment

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

No, it was, I suggested this change, but he missed the leading dot, so ./.pants.bootstrap.

Copy link
Member

Choose a reason for hiding this comment

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

Or just .pants.bootstrap..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops. sorry folks

Copy link
Member

Choose a reason for hiding this comment

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

@michaelglass no worries, it was a double-miss between me and Eric :)

Copy link
Member

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@@ -233,8 +233,7 @@ This can be fixed by setting `ulimit -n 10000`. (10,000 should work in all cases
> We recommend permanently setting this by either:
>
> 1. Adding `ulimit -n 10000` to your `./pants` script.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, minor nit, there's a missing leading . on the file name :)

@Eric-Arellano
Copy link
Contributor

Thanks for making Pants better!

@Eric-Arellano Eric-Arellano merged commit 6c2be26 into pantsbuild:main Nov 17, 2022
@michaelglass michaelglass deleted the patch-1 branch November 17, 2022 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants