-
-
Notifications
You must be signed in to change notification settings - Fork 13k
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
pcsclite: Remove the confdir configure option (fix #121088) #121247
base: master
Are you sure you want to change the base?
Conversation
The
This PR doesn't address either issue. |
Result of 297 packages marked as broken and skipped:
1 package failed to build:742 packages skipped due to time constraints:
50 packages built successfully:
2 suggestions:
Note that build failures may predate this PR, and could be nondeterministic or hardware dependent. Result of 293 packages marked as broken and skipped:
688 packages skipped due to time constraints:
50 packages built successfully:
|
Indeed, but its usage remains incorrect, AFAICT. As per
I cannot reproduce this behavior with my patch applied.
How do you observe that? The default value for
This is not the point of this PR. I've addressed the PolicyKit issue in #121246. |
b81b9ba
to
05e9ea3
Compare
The issue here is that while the default confdir is So the real fix would be to change it to that instead. Thanks for your patience @thblt - do you mind revising this PR? |
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? |
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.
|
05e9ea3
to
8261809
Compare
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. |
It should be correct, the |
8261809
to
21c3628
Compare
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.
21c3628
to
7460ef9
Compare
I marked this as stale due to inactivity. → More info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
Reported upstream: LudovicRousseau/PCSC#115. |
Upstream docs have been updated and they now indicate that |
@peterhoeg @kirelagin @thblt what's the state of this PR? Has this been solved elsewhere in the meantime? |
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:
But it was set as:
(Also a very minor formatting change that Emacs insisted to make and I forgot to unstage.)
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)