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

Allow user to specify the target directory #255

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eddsalkield
Copy link
Contributor

A new default target can be set using:

  • A target option in rcrc(5)
  • -T flag
    A per-dotfiles target can be set using a target manifest file.

Provides a solution to issue #246.

A new default target can be set using:
* A target option in rcrc(5)
* -T flag

A per-dotfiles target can be set using a target manifest file
@eddsalkield
Copy link
Contributor Author

A few things to discuss: expanding environment variables in the target manifest file is currently handled safely by envsubst, which adds a dependency on GNU gettext. I believe this is the right approach, since GNU utilities are very portable and standardised.

If this dependency is a problem, however, I'm sure we can make it work by passing the internals to exec instead. It's a bit hacky and unsafe, but would do the trick without any additional dependencies.

@mike-burns
Copy link
Contributor

OK this diff isn't so bad -- we might have a good compromise here. I'll have to review it more carefully, but in the meantime I want to pop in to say: thank you!

@mike-burns mike-burns deleted the branch thoughtbot:master March 4, 2022 19:48
@mike-burns mike-burns closed this Mar 4, 2022
@mike-burns mike-burns reopened this Mar 4, 2022
@uzxmx
Copy link

uzxmx commented May 20, 2022

Any progress on this PR? Really hope this can be merged soon.

@Amir-Ahmad
Copy link

This PR would add alot of value for me.

@mike-burns
Copy link
Contributor

Cool implementation, thank you for this! Hope you're still around.

I believe this is the right approach, since GNU utilities are very portable and standardised.

Most of my computers don't have much GNU code on them, so I'd prefer to find something in POSIX that we can rely on. I don't see envsubst in the OpenBSD man pages, for example. (It can be installed as a package, but at that point I'd just have people install a Rust compiler or whatever and stop maintaining painful shell code.)


This adds a new metafile, target, which I don't love. Is the use case real -- do you actually have one dotfiles dir that targets HOME and another that targets XDG_CONFIG_DIR?

@eddsalkield
Copy link
Contributor Author

Yep, still around, although admittedly not using rcm as much at the moment and I have not very much availability to tweak this patch.

Rethinking this through, I still think the patch is useful but I'm less convinced that the target meta-file is absolutely necessary. This is because the user can just maintain one rcm directory for each target. The behaviour of checking multiple directories and falling back to the default isn't really necessary either since the user could do something like ${XDG_CONFIG_HOME:-$HOME/.config}.

I agree that envsubst isn't really too necessary. The -T command line option doesn't need to expand the environment variable, since the user's shell can probably do that for them. So the only place this is required to expand the target in the rcrc config file.

To drop the dependency, somebody needs to hack together a POSIX-compliant implementation in pure sh. Note that it'll probably be less secure since it might depend on exec.

jonthn pushed a commit to jonthn/rcm that referenced this pull request May 7, 2023
A new default target can be set using:
* A target option in rcrc(5)
* -T flag

(based on the work in thoughtbot#255) for thoughtbot#246
jonthn pushed a commit to jonthn/rcm that referenced this pull request May 8, 2023
A new default target can be set using:
* A target option in rcrc(5)
* -T flag

(based on the work in thoughtbot#255) for thoughtbot#246
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.

4 participants