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

Look for ~/.config/avrdude/config configuration file #1131

Merged
merged 5 commits into from
Oct 23, 2022

Conversation

steelman
Copy link
Contributor

Traditionally per-user configuration files have been placed in user's home directory with their names beginnig with a dot to hide them from some tools like ls(1). However, the number of programs following this convention have grown over time to the point where the number of hidden files becomes inconvenient to some users. For this reason the XDG Base Directory Specification[1] specifies an alternate place to store configuration files under ~/.config directory.

This patch enables avrdude to look for ~/.config/avrdude/config configuration file, if ~/.avrduderc doesn't exist.

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.8.html

Traditionally per-user configuration files have been placed
in user's home directory with their names beginnig with a dot
to hide them from some tools like ls(1). However, the number
of programs following this convention have grown over time
to the point where the number of hidden files becomes inconvenient to
some users. For this reason the XDG Base Directory Specification[1]
specifies an alternate place to store configuration files under
~/.config directory.

This patch enables avrdude to look for ~/.config/avrdude/config
configuration file, if ~/.avrduderc doesn't exist.

[1] https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.8.html
@mcuee mcuee added the enhancement New feature or request label Oct 18, 2022
@stefanrueger
Copy link
Collaborator

stefanrueger commented Oct 18, 2022

@steelman Thanks for a useful and well-written contribution, espec also for providing documentation. I tested this and reviewed the source and the manual. All good, but I will suggest a few minor changes to the documentation via pushing a small commit onto your branch. One question: how welded are you to the basename config of the path ~/.config/avrdude/config? I have not seen a project name its configuration file config, and we have so far used avrdude.conf, .avrduderc or avrdude.rc. Can I suggest either avrdude.rc or avrduderc. Unless others (@dl8dtl @mariusgreuel @MCUdude @mcuee?) have a preference I ever so slightly lean to avrdude.rc in the ~/.config/avrdude/ directory.

The unsuccessful check in this PR is probably a red herring (some timeout in the gitlab runners?). We'll know when the checks are run again.

@mcuee
Copy link
Collaborator

mcuee commented Oct 18, 2022

I think avrdude.rc is a good one. avrduderc is also okay for me.

@steelman
Copy link
Contributor Author

how welded are you to the basename config

Technically not at all and I understand your question as many programs which moved their files to ~/.config faced this question and gave different answers. The way I think about it is: ofc no point in starting with a dot; avrduderc or avrdude.rc aren't bad as such but the name of the program is already there in the name of the directory, so why repeat it? On the other hand, the avrdude package may provide more tools in the future which will store their configs in the same directory. On the third hand… the "rc" somewhat reminds me of scripts (as in BSD) and the config file isn't a script at all. I am not sure.

How about avrdude.conf same as system wide?

@stefanrueger
Copy link
Collaborator

The code does not safely concatenate file names for the config file (this problem existed before).

  • Rectified in the commit that I just pushed onto this branch
  • Also changed the alternative config file name to avrdude/avrdude.rc, which is either searched in ${XDG_CONFIG_HOME} or, if that's not there, in ~/.config/ (all of this if ~/.avrduderc does not exist --- this PR is backward compatible in that ~/.avrduderc is the preferred local config file)
  • Updated the documentation correspondingly

Once, again, many thanks @steelman --- could you please test this?

@MCUdude
Copy link
Collaborator

MCUdude commented Oct 18, 2022

I think avrdude.rc is a good one. avrduderc is also okay for me.

How about supporting both in ~/.config ?

@stefanrueger
Copy link
Collaborator

How about avrdude.conf same as system wide?

No, the system-wide configuration file contains the full data, whereas .avrduderc is for modifications/new parts/new programmers that the user wishes to put in place. Yes, you are correct, rc originally used to stand for run commands, but it's been used for decades for all sorts of runtime configurations or, as I think of it in the context of AVRDUDE, residual configuration :)

Anyway, the name is now defined symbolically in
https://github.com/steelman/avrdude/blob/482198431c4174b2ed2426349d301c03aca4cddf/src/avrdude.h#L29

We can (and will) change it once there is a consensus.

@mcuee
Copy link
Collaborator

mcuee commented Oct 18, 2022

I think avrdude.rc is a good one. avrduderc is also okay for me.

How about supporting both in ~/.config ?

Okay to me as well.

@stefanrueger
Copy link
Collaborator

How about supporting both in ~/.config ?

Really better if it's only one name. Defining an alterative XDG_USER_CONF and then checking in a certain order first in ${XDG_CONFIG_HOME} and then also in ~/.config and explaining all in avrdude.1 and avrdude.texi is just a headache too far...

@steelman
Copy link
Contributor Author

Looks good to me.

@dl8dtl
Copy link
Contributor

dl8dtl commented Oct 18, 2022

I'd even go as far as preferring the new location, and maintaining the old one (until AVRDUDE v8 is released) just as a fallback.

I think systems like Windows don't particularly like filenames without a suffix, so I wouldn't pick avrduderc. avrdude.conf is fine with me as well as avrdude.rc.

@stefanrueger
Copy link
Collaborator

I'd even go as far as preferring the new location, and maintaining the old one (until AVRDUDE v8 is released) just as a fallback.

This would make the current code/order change from
https://github.com/steelman/avrdude/blob/482198431c4174b2ed2426349d301c03aca4cddf/src/main.c#L881-L885
to sth like

  if(!concatpath(usr_config, getenv("XDG_CONFIG_HOME"), XDG_USER_CONF_FILE, sizeof usr_config))
    concatpath(usr_config, getenv("HOME"), ".config/" XDG_USER_CONF_FILE, sizeof usr_config);
  if(stat(usr_config, &sb) < 0 || (sb.st_mode & S_IFREG) == 0)
    concatpath(usr_config, getenv("HOME"), USER_CONF_FILE, sizeof usr_config);

@stefanrueger
Copy link
Collaborator

So, this needs deciding whether to go with @dl8dtl's suggestion of giving preference of new location over traditional (happy with that propsal) and what the name should be. Then final testing and it's good to merge.

@mcuee
Copy link
Collaborator

mcuee commented Oct 19, 2022

I am okay with @dl8dtl's suggestion. As for the name. if we need to pick only one from avrdude.conf and avrdude.rc, I actually prefer avrdude.rc to differentiate a bit from the system wide avrdude.conf.

@stefanrueger stefanrueger linked an issue Oct 19, 2022 that may be closed by this pull request
@stefanrueger
Copy link
Collaborator

OK, have changed the order to be xdg-style avrdude.rc file first before fallback ~/.avrduderc and have tested. Also corrected the location and description of avrdude.pdf in avrdude.1.

I consider this ready to squash and merge.

@steelman
Copy link
Contributor Author

What's your workflow? Am I supposed to it? (Like, I am fine with it, I am asking because this is my first contribution to avrdude)

@stefanrueger
Copy link
Collaborator

stefanrueger commented Oct 19, 2022

Nothing more to do for you, @steelman. Thanks for the idea, initial implementation and contributing to the discussion.

We normally let PRs hang around for a couple of days, and then when they are considered ready, there is sometimes another wait until a few ones have batched up that are merged at the same time (it's easier that way).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Location for avrdude.pdf
5 participants