8000 Use XDG_STATE_HOME environment variable as a preference above HOME by jaapjansma · Pull Request #243 · civicrm/cv · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Use XDG_STATE_HOME environment variable as a preference above HOME #243

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Apr 5, 2025
Merged

Use XDG_STATE_HOME environment variable as a preference above HOME #243

merged 6 commits into from
Apr 5, 2025

Conversation

jaapjansma
Copy link
Contributor

Fix for #195

One can set the environment variable XDG_STATE_HOME
This will take precedence over the HOME environment variable.

@totten
Copy link
Member
totten commented Mar 19, 2025

Thanks for moving this forward, @jaapjansma!

The test-failures seem to be some unrelated issue in master. (I haven't yet figured out a sufficient routine to focus-in on the problem - but will look at it.)

I wonder if you've had a chance to read the definitions from https://specifications.freedesktop.org/basedir-spec/latest/#variables -- there are a few variables, and the choice of vars somewhat subjective (but not completely).

Like here's my impression

  • UpgradeDbCommand::getUpgradeFile() - Sounds like closest fit to XDG_STATE_HOME?
  • Config::getFileName() and CvPlugins - These sound like XDG_CONFIG_HOME might be closer fit?

@jaapjansma
Copy link
Contributor Author

I agree with you for following the standard. However using cv on a well used hosting platform as CiviHosting one needs to set two environment variables to get cv working. Because on CiviHosting those environment variables are not set.

So this requires additional documentation and makes the tool probably less user friendly.

@jaapjansma
Copy link
Contributor Author

I have updated the PR to reflect the standard of XDG Base Directories

@totten
Copy link
Member
totten commented Apr 5, 2025

Great. Thanks for moving this forward @jaapjansma.

I'm adding a revision (and plan to merge assuming tests pass) to try to solve a little puzzle:

  • XDG vars should take priority - but historically, they haven't been factor.
  • Some people already have ~/.cv.json
  • It's hard to say when, exactly, the XDG vars are preset. (Ex: In my current Ubuntu version, the distro presents ~8 XDG vars -- but not specifically XDG_CONFIG_HOME.)
  • If one already has ~/.cv.json, and if one already has XDG vars, then the revision could block loading of the (previously-valid) ~/.cv.json.

So with the new commit, it will check a prioritized list of candidates (XDG_CONFIG_HOME/.cv.json then HOME/.cv.json) and use an existence-check. This should mean that:

  • If you already have .cv.json (in either location), then it will prefer to use the existing file (wherever that is).
  • If you don't already have .cv.json, it will use the first plausible location (either XDG_CONFIG_HOME or HOME).

(For the upgrade-log case -- the data is temporary/transactional, so it doesn't matter if the path switches...)

@totten totten merged commit b460cdd into civicrm:master Apr 5, 2025
1 check passed
@demeritcowboy
Copy link
Contributor

It seems this just broke CiviCarrot. My initial guess is that cv vars:fill now creates/fills something other than ~/.cv.json, but carrot's assumption was that it would be ~/.cv.json. i.e. in github actions XDG_CONFIG_HOME may not be ~

Assuming this is true, I don't know how many other environments assume ~ is the same as XDG_CONFIG_HOME.

@demeritcowboy
Copy link
Contributor

This is an easy update for carrot, and it's running now. But as noted I don't know how many other environments's scripts are unaware the XDG stuff exists (I wasn't), and just use ~.

@totten
Copy link
Member
totten commented Apr 5, 2025

@demeritcowboy Ah, thanks for reporting back.

carrot's assumption was that it would be ~/.cv.json

So the implication is that you automatically spin-up a new system and have some other script-y-bits which manipulate the .cv.json? Along the lines of (pseudocode)...

function on_create_new_system() {
  ...
  git clone https://github.com/civicrm/civicrm-core
  ...
  cv core:install ...
  ...
  cv vars:fill
  jq '.sites["/my/site1"].SOME_FIELD = "newvalue"'  ~/.cv.json | sponge ~/.cv.json
  ...
}

If I understand -- #243 should be OK if you manually use vars:fill and manually edit the displayed JSON file -- the problem is if vars:fill is part of a script (which isn't something I ever considered doing - but I can see how it fits for carrot). Possible reactions are like:

  • (1) Update script -- Pre-configure CV_CONFIG=$HOME/.cv.json, or...
  • (2) Update script -- Pre-create ~/.cv.json as an empty placeholder, or...
  • (3) Update script -- Parse the output of cv vars:fill to determine the real filename, or...
  • (4) Update script -- Pass the data to cv vars:fill (ex: echo '{"FOO":"bar"}' | cv vars:fill --file=/dev/stdin) so that you don't need to touch the file directly.
    • (4b) (Or if "fill" behavior isn't quite the right fit, then we could update both cv and the script -- add a subcommand like cv vars:set FOO=bar.)
  • (5) Partial-revert -- keep the parts which use XDG_STATE_HOME for upgrade-logs (needed for Allow upgrades when HOME is read-only #195) but not the part for XDG_CONFIG_HOME (.cv.json).

Unfortunately, it's hard to tell if there other scripts that call vars:fill. FWIW, in universe, I don't see any other scripted references to vars:fill or cv.json. (The search covers a handful of docker projects, and there area couple dozen extensions with their own .github/ or .gitlab/.)

Maybe we hold as-is for the moment -- but if someone comes along to comment on a second script that's impacted, then we can consider one of the other interventions?

@demeritcowboy
Copy link
Contributor

Carrot basically follows the dev docs setup steps: https://docs.civicrm.org/dev/en/latest/testing/#setup

$ cv vars:fill
$ vi ~/.cv.json

just obviously doesn't use vi.

So where vars:fill creates the file has potentially changed, and if you're unaware the XDG vars are set on your system or even exist then that description is no longer correct.

@jaapjansma
Copy link
Contributor Author

Maybe we can show the path upon cv vars:show command, is that an idea and then update the docs?

@totten
Copy link
Member
totten commented Apr 8, 2025

Yeah, I think adding some more commands like cv vars:set or cv vars:show would make sense for that kind of semi-automated configuration -- so that you can script it without needing to know the specific file-format. (Similar how to composer allows you to do scripted configuration via composer config -- you usually don't need scripts to edit composer.json.)

Stepping back for a second -- we also now have site-aliases (#240). In a strict technical sense, .cv.json and site-aliases are completely separate things which...

  • Track different variables (_CV[DEMO_PASS] vs SERVER[HTTP_HOST])
  • Load at different moments, in different layers
  • Read from different sources (~/.cv.json vs ~/.cv/alias/*.json) with different keys ($config['sites']['/path/to/civicrm.settings.php'] vs @aliasname)
  • And basically address different problems (phpunit vs sysadmin).

And yet... conceptually... they look really, really similar (stored config-vars keyed by a site-identifier) and have some similar workflow needs (needing helpers for interactive-setup or scripted-setup). I suspect there will be some way to reconcile them into the same mechanism... but it's probably a multiple-step process (not planned or scheduled right now).

My point is that ~/.cv.json may change more in the future. There are ways to deal with that:

  • Be lazy. Don't do anything more to .cv.json until there's a clearer picture on whether/how to unify with aliases.
  • Be active. Add some more commands (cv vars:set) to abstract away from the format. (When changes come, we can edit commands to support new formats.)

@jaapjansma jaapjansma mentioned this pull request Apr 10, 2025
Merged
@jaapjansma
Copy link
Contributor Author
jaapjansma commented Apr 10, 2025 •
edited
Loading

I did a follow up PR showing the cv.json file location when running cv vars:show. See #248

@totten totten mentioned this pull request Apr 22, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jaapjansma @totten @demeritcowboy
0