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

env_config: add HOMEBREW_BUNDLE_USER_CACHE. #17327

Merged
merged 1 commit into from
May 17, 2024

Conversation

MikeMcQuaid
Copy link
Member

Used to configure BUNDLE_USER_CACHE.

Changes approach in #17324 where I saw some local issues if e.g. the directory wasn't created or had weird permissions.

CC @dkav that you'll need to set this variable now.

@carlocab
Copy link
Member

CC @dkav that you'll need to set this variable now.

Won't be needed for users who set BUNDLE_USER_CACHE; brew will use that env variable to set the HOMEBREW_ version

Used to configure `BUNDLE_USER_CACHE`.
@@ -91,6 +91,9 @@ module EnvConfig
description: "Use this as the browser when opening project homepages.",
default_text: "`$BROWSER` or the OS's default browser.",
},
HOMEBREW_BUNDLE_USER_CACHE: {
description: "If set, use this directory as the `bundle`(1) user cache.",
Copy link
Member

Choose a reason for hiding this comment

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

Should we default to a location inside HOMEBREW_CACHE? Otherwise we would write to HOME, which is weird.

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 don't understand the request, asking for a HOMEBREW_CACHE default is just asking for this PR to be closed and keep #17324 functionality?

I don't think we should have or need a default at all. The existing logic was fine for everyone except dkav.

Alternatively: we rip out all this logic and make #17276 a WONTFIX. Really not convinced this is worth the amount of effort that's gone into it and definitely not worth any more.

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the request, asking for a HOMEBREW_CACHE default is just asking for this PR to be closed and keep #17324 functionality?

Not necessarily; we can simply add a default: key here. Will make an in-line suggestion below.

I don't think we should have or need a default at all. The existing logic was fine for everyone except dkav.

Ok, that's fine! I just agreed with you when you said not writing to HOME would be ideal. But I'm not hugely bothered by it, so we can leave this as it is.

Copy link
Member

@carlocab carlocab May 17, 2024

Choose a reason for hiding this comment

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

Suggested change
description: "If set, use this directory as the `bundle`(1) user cache.",
description: "If set, use this directory as the `bundle`(1) user cache.",
default: "#{HOMEBREW_DEFAULT_CACHE}/bundler_cache",

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 just agreed with you when you said not writing to HOME would be ideal. But I'm not hugely bothered by it, so we can leave this as it is.

Sorry, yeh, provided insufficient context here: there's additional complexity from not writing to HOME e.g. permissions, directory not existing, etc. that means that I think it's not worthwhile given the level of demand we've seen (one person 😅)

@@ -91,6 +91,9 @@ module EnvConfig
description: "Use this as the browser when opening project homepages.",
default_text: "`$BROWSER` or the OS's default browser.",
},
HOMEBREW_BUNDLE_USER_CACHE: {
description: "If set, use this directory as the `bundle`(1) user cache.",
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand the request, asking for a HOMEBREW_CACHE default is just asking for this PR to be closed and keep #17324 functionality?

Not necessarily; we can simply add a default: key here. Will make an in-line suggestion below.

I don't think we should have or need a default at all. The existing logic was fine for everyone except dkav.

Ok, that's fine! I just agreed with you when you said not writing to HOME would be ideal. But I'm not hugely bothered by it, so we can leave this as it is.

@@ -91,6 +91,9 @@ module EnvConfig
description: "Use this as the browser when opening project homepages.",
default_text: "`$BROWSER` or the OS's default browser.",
},
HOMEBREW_BUNDLE_USER_CACHE: {
description: "If set, use this directory as the `bundle`(1) user cache.",
Copy link
Member

@carlocab carlocab May 17, 2024

Choose a reason for hiding this comment

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

Suggested change
description: "If set, use this directory as the `bundle`(1) user cache.",
description: "If set, use this directory as the `bundle`(1) user cache.",
default: "#{HOMEBREW_DEFAULT_CACHE}/bundler_cache",

@MikeMcQuaid MikeMcQuaid merged commit b8d844c into master May 17, 2024
26 checks passed
@MikeMcQuaid MikeMcQuaid deleted the bundle_user_cache_again branch May 17, 2024 15:29
@MikeMcQuaid
Copy link
Member Author

Thanks @carlocab!

@github-actions github-actions bot added the outdated PR was locked due to age label Jun 18, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 18, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants