pcsclite: Remove the confdir configure option (fix #121088) #121247

thblt · 2021-04-30T08:42:06Z

Motivation

This reverts commit 6d23cfd by @peterhoeg, which solved #121088 by hardcoding the configuration file path into the binary call. Instead, it removes the --enable-confdir build option which was the root cause of the issue. To clarify, this is how ./configure --help describes the parameter:

  --enable-confdir=DIR directory containing reader
                       configurations (default /etc/reader.conf.d)

But it was set as:

  configureFlags = [
     "--enable-confdir=/etc" … ]

(Also a very minor formatting change that Emacs insisted to make and I forgot to unstage.)

Things done

peterhoeg · 2021-04-30T09:34:28Z

The enable-confdir was there all along - even before #97440. The issues are:

pcsclite goes off on its own if it cannot read proper config from /etc/reader.conf and tries everything else in that directory. This is just bananas. If we don't specify it, it will default to $out/etc which while it doesn't blow up, is just pointless. The best way to deal with this is just to specify a file inside nix store which we generate and then reference that in ExecStart (this way we can also drop the restartTrigger).
the policykit files aren't registered properly.

This PR doesn't address either issue.

r-rmcgibbo · 2021-04-30T09:50:36Z

Result of nixpkgs-review pr 121247 at b81b9ba6 run on x86_64-linux 1

297 packages marked as broken and skipped:

globalplatform
gnokii
gnome3.gnome-books
gnome3.gnome-documents
gnuradio3_7Packages.gsm
gppcscconnectionplugin
gpshell
hash-slinger
libsForQt512.akonadi
libsForQt512.akonadi-calendar
...

1 package failed to build:

moneyplex

742 packages skipped due to time constraints:

AusweisApp2
acsccid
adapta-gtk-theme
aeon
aerc
afew
ajour
almanah
alot (python38Packages.alot)
amarok (amarok-kf5)
...

50 packages built successfully:

cosign
gcr (gnome3.gcr)
git-remote-gcrypt
git-secret
gitRepo
gnome-online-accounts (gnome3.gnome-online-accounts ,gnome3.gnome_online_accounts)
gvfs (gnome2.gvfs)
libgdata (gnome3.libgdata)
gnupg (gnupg22)
gnupg1 (gnupg1compat)
gpgme
hexio
keychain
leiningen
libblockdev
libnma
libsForQt5.grantleetheme (libsForQt515.grantleetheme ,plasma5Packages.grantleetheme)
libsForQt5.kaccounts-integration (libsForQt515.kaccounts-integration ,plasma5Packages.kaccounts-integration)
libsForQt5.kcmutils (libsForQt515.kcmutils ,plasma5Packages.kcmutils)
libsForQt5.kdeclarative (libsForQt515.kdeclarative ,plasma5Packages.kdeclarative)
libsForQt5.kdesignerplugin (libsForQt515.kdesignerplugin ,plasma5Packages.kdesignerplugin)
libsForQt5.kio (libsForQt515.kio ,plasma5Packages.kio)
libsForQt5.knewstuff (libsForQt515.knewstuff ,plasma5Packages.knewstuff)
libsForQt5.knotifyconfig (libsForQt515.knotifyconfig ,plasma5Packages.knotifyconfig)
libsForQt5.kpimtextedit (libsForQt515.kpimtextedit ,plasma5Packages.kpimtextedit)
libsForQt5.kwallet (libsForQt515.kwallet ,plasma5Packages.kwallet)
libsForQt5.qgpgme (libsForQt515.qgpgme ,plasma5Packages.qgpgme)
libsForQt514.kcmutils
libsForQt514.kdeclarative
libsForQt514.kio
libsForQt514.kwallet
libsForQt514.qgpgme
nice-dcv-client
open-ecard
pass
pcsclite
perl530Packages.pcscperl
perl532Packages.GnuPGInterface
perl532Packages.pcscperl
python38Packages.btchip
python38Packages.emv
python38Packages.pypass
python38Packages.pysatochip
python38Packages.pyscard
python39Packages.emv
python39Packages.pysatochip
python39Packages.pyscard
udisks (udisks2)
volume_key
yadm

2 suggestions:

warning: missing-patch-comment
Please add a comment on the line above, explaining the purpose of this patch.
Near pkgs/tools/security/pcsclite/default.nix:25:15:
```
   |
25 |   patches = [ ./no-dropdir-literals.patch ];
   |               ^
```
warning: maintainers-missing
Package does not have a maintainer. Consider adding yourself?

Near pkgs/tools/security/pcsclite/default.nix:63:3:
```
   |
63 |   meta = with lib; {
   |   ^
```

Note that build failures may predate this PR, and could be nondeterministic or hardware dependent.
Please exercise your independent judgement.

Result of nixpkgs-review pr 121247 at b81b9ba6 run on aarch64-linux 1

293 packages marked as broken and skipped:

globalplatform
gnokii
gnome3.gnome-books
gnome3.gnome-documents
gnuradio3_7Packages.gsm
gppcscconnectionplugin
gpshell
kstars
libsForQt512.akonadi
libsForQt512.akonadi-calendar
...

688 packages skipped due to time constraints:

AusweisApp2
acsccid
adapta-gtk-theme
aerc
afew
ajour
almanah
alot (python38Packages.alot)
appimagekit
aptly
...

50 packages built successfully:

cosign
gcr (gnome3.gcr)
git-remote-gcrypt
git-secret
gitRepo
gnome-online-accounts (gnome3.gnome-online-accounts ,gnome3.gnome_online_accounts)
gvfs (gnome2.gvfs)
libgdata (gnome3.libgdata)
gnupg (gnupg22)
gnupg1 (gnupg1compat)
gpgme
hexio
keychain
leiningen
libblockdev
libnma
libsForQt5.grantleetheme (libsForQt515.grantleetheme ,plasma5Packages.grantleetheme)
libsForQt5.kaccounts-integration (libsForQt515.kaccounts-integration ,plasma5Packages.kaccounts-integration)
libsForQt5.kcmutils (libsForQt515.kcmutils ,plasma5Packages.kcmutils)
libsForQt5.kdeclarative (libsForQt515.kdeclarative ,plasma5Packages.kdeclarative)
libsForQt5.kdesignerplugin (libsForQt515.kdesignerplugin ,plasma5Packages.kdesignerplugin)
libsForQt5.kio (libsForQt515.kio ,plasma5Packages.kio)
libsForQt5.knewstuff (libsForQt515.knewstuff ,plasma5Packages.knewstuff)
libsForQt5.knotifyconfig (libsForQt515.knotifyconfig ,plasma5Packages.knotifyconfig)
libsForQt5.kpimtextedit (libsForQt515.kpimtextedit ,plasma5Packages.kpimtextedit)
libsForQt5.kwallet (libsForQt515.kwallet ,plasma5Packages.kwallet)
libsForQt5.qgpgme (libsForQt515.qgpgme ,plasma5Packages.qgpgme)
libsForQt514.kcmutils
libsForQt514.kdeclarative
libsForQt514.kio
libsForQt514.kwallet
libsForQt514.qgpgme
nice-dcv-client
open-ecard
pass
pcsclite
perl532Packages.GnuPGInterface
perl532Packages.pcscperl
python38Packages.btchip
python38Packages.emv
python38Packages.pypass
python38Packages.pysatochip
python38Packages.pyscard
python39Packages.btchip
python39Packages.emv
python39Packages.pysatochip
python39Packages.pyscard
udisks (udisks2)
volume_key
yadm

thblt · 2021-04-30T09:55:59Z

The enable-confdir was there all along - even before #97440. The issues are:

Indeed, but its usage remains incorrect, AFAICT. As per configure --help, it's meant to tell pcsclite which directory contains all its config files, and only its config files. This cannot be /etc.

pcsclite goes off on its own if it cannot read proper config from /etc/reader.conf and tries everything else in that directory. This is just bananas.

I cannot reproduce this behavior with my patch applied.

If we don't specify it, it will default to $out/etc which while it doesn't blow up, is just pointless.

How do you observe that? The default value for confdir is /etc/reader.conf.d, which makes perfect sense.

the policykit files aren't registered properly.

This is not the point of this PR. I've addressed the PolicyKit issue in #121246.

pkgs/tools/security/pcsclite/default.nix

peterhoeg · 2021-05-06T00:50:36Z

The issue here is that while the default confdir is /etc/reader.conf.d if not specified, when we're building it with the nix, that gets set to $out/etc/reader.conf.d.

So the real fix would be to change it to that instead. Thanks for your patience @thblt - do you mind revising this PR?

thblt · 2021-05-06T11:52:21Z

Ha, right, thank you. Just one question, do we actually need confdir to be set to anything? Is it for Nix on non-NixOS platforms?

peterhoeg · 2021-05-06T14:23:07Z

Is it for Nix on non-NixOS platforms?

That's the most important reason, yes. Another is that it's very convenient to have a writeable place where you can just drop files in order to try things out without having to build a new generation.

thblt · 2021-05-07T07:22:24Z

Done, but I'm not sure it'll work — this is the default value, after all, I'm afraid $OUT is implied. I'm rebuilding to check.

thblt · 2021-05-07T07:25:50Z

It should be correct, the configure scripts actually defaults to confdir="${sysconfdir}/reader.conf.d"

This reverts commit 6d23cfd, which solved NixOS#121088 by hardcoding the configuration file path into the binary call. Instead, it sets the --enable-confdir build option which was the root cause of the issue. Please see the comment in default.nix for more details about this option.

stale · 2021-11-09T19:20:28Z

I marked this as stale due to inactivity. → More info

kirelagin

This looks good and makes perfect sense.

While we are at it, would it make sense to also remove

environment.etc."reader.conf".source = cfgFile;

? I don’t think it serves any purpose and is thus a bit confusing.

kirelagin

Wait, no, sorry, I think we need to keep the -c option though? If I am reading the code correctly, if -c is given, the daemon will go read config from this file, but if it is not given, it will go and read everything from confdir.

So, if you remove this option, then readerConfig written by the NixOS module will not be used in any way.

I think the confusion comes from the pcsclite documentation being entirely wrong and misleading in that /etc/reader.conf has no special significance for the code and won’t be ever tried, unless explicitly given as -c.

kirelagin · 2021-12-04T15:56:40Z

Reported upstream: LudovicRousseau/PCSC#115.

kirelagin · 2021-12-05T01:33:49Z

Upstream docs have been updated and they now indicate that -c accepts a directory rather than a file (and, I think, the fact that passing a file works is an undocumented feature/coincidence), so I think we should switch to writeTextFile with destination set instead of writeText

flokli · 2024-04-19T20:29:30Z

@peterhoeg @kirelagin @thblt what's the state of this PR? Has this been solved elsewhere in the meantime?

github-actions bot added 6.topic: nixos 8.has: module (update) labels Apr 30, 2021

ofborg bot added 10.rebuild-darwin: 11-100 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 labels Apr 30, 2021

SuperSandro2000 reviewed Apr 30, 2021

View reviewed changes

pkgs/tools/security/pcsclite/default.nix Outdated Show resolved Hide resolved

thblt force-pushed the fix-for-the-pcsclite-fix branch from b81b9ba to 05e9ea3 Compare April 30, 2021 11:21

This was referenced Apr 30, 2021

pcscd: fails to start #121088

Closed

nixos/pcscd: ensure polkit rules are loaded (fix #121121) #121246

Merged

thblt force-pushed the fix-for-the-pcsclite-fix branch from 05e9ea3 to 8261809 Compare May 7, 2021 07:18

thblt force-pushed the fix-for-the-pcsclite-fix branch from 8261809 to 21c3628 Compare May 7, 2021 07:45

thblt force-pushed the fix-for-the-pcsclite-fix branch from 21c3628 to 7460ef9 Compare May 7, 2021 07:46

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 9, 2021

kirelagin approved these changes Dec 4, 2021

View reviewed changes

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 4, 2021

kirelagin suggested changes Dec 4, 2021

View reviewed changes

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 12, 2022

Artturin marked this pull request as draft July 28, 2023 00:13

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 28, 2023

wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict labels Mar 19, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pcsclite: Remove the confdir configure option (fix #121088) #121247

pcsclite: Remove the confdir configure option (fix #121088) #121247

thblt commented Apr 30, 2021

peterhoeg commented Apr 30, 2021

r-rmcgibbo commented Apr 30, 2021 •

edited

thblt commented Apr 30, 2021

peterhoeg commented May 6, 2021

thblt commented May 6, 2021

peterhoeg commented May 6, 2021 via email

thblt commented May 7, 2021

thblt commented May 7, 2021

stale bot commented Nov 9, 2021

kirelagin left a comment

kirelagin left a comment

kirelagin commented Dec 4, 2021

kirelagin commented Dec 5, 2021

flokli commented Apr 19, 2024

pcsclite: Remove the confdir configure option (fix #121088) #121247

Are you sure you want to change the base?

pcsclite: Remove the confdir configure option (fix #121088) #121247

Conversation

thblt commented Apr 30, 2021

Motivation

Things done

peterhoeg commented Apr 30, 2021

r-rmcgibbo commented Apr 30, 2021 • edited

thblt commented Apr 30, 2021

peterhoeg commented May 6, 2021

thblt commented May 6, 2021

peterhoeg commented May 6, 2021 via email

thblt commented May 7, 2021

thblt commented May 7, 2021

stale bot commented Nov 9, 2021

kirelagin left a comment

Choose a reason for hiding this comment

kirelagin left a comment

Choose a reason for hiding this comment

kirelagin commented Dec 4, 2021

kirelagin commented Dec 5, 2021

flokli commented Apr 19, 2024

r-rmcgibbo commented Apr 30, 2021 •

edited