Closed jhiemstrawisc closed 1 year ago
Suddenly I have opinions about allowing the robot to fix, and not just complain about, linter issues!
I would like to make this thread safe. Can you please replace the the storage of this information with a std::shared_ptr
Hmm, any ideas what might be causing the EnforcerTest
to fail in the one instance? When I run locally, all tests pass.
You say that, but now it looks good. Are you happy with this pull request now?
@jhiemstrawisc - can you clean up the remaining linter items? Once that's done, I think we're ready to go.
@bbockelm Okay, latest commit made the linter happy, and all the tests are passing.
The symbols config_set_str() and friends seem far too generic for a public library with C bindings.
With the new config functions prefixed with scitokens_
, I suggest changing the unprefixed versions to just call the prefixed ones. That avoids a clone of the code, where the two copies can diverge in functionality by accident.
Anything else on this, or are we ready to wrap things up?
Nothing from my end.
I wonder how fast we can depreciate the config_* functions. Is HTCondor already using them? I know xrootd isn't. If no one is using them, can we just remove them already?
Good point. The config_set_str() and config_get_str() were never released. Why release a deprecated API?
HTCondor does not use config_set_int() or config_get_int().
From HTCondor's point of view, it could be removed immediately.
I just noticed that the other functions start with scitoken_
, not scitokens_
. Most, though not all, are dealing with a single SciToken object.
Oh, I thought I had heard that the older config_set/get_int
functions were already in use by someone somewhere. If nobody is using them at this point, I'll update to just get rid of them.
I think that Brian B wanted to have the C++ deprecated decorator (not just a comment).
https://en.cppreference.com/w/cpp/language/attributes/deprecated
Since scitokens.h contains C declarations, I don't think the C++ deprecated decorator is appropriate (or even allowed).
Brian B said to worry about any deprecated decorators in another PR at another time. I think he's looking to have this PR wrapped up expediently.
It looks like the deprecated decorator for C is supported after C23: https://en.cppreference.com/w/c/language/attributes/deprecated, whereas the deprecated decorator for C++ has been supported since C++14.
Yeah - unfortunately, we’ll need to wrap the deprecated stuff in some macro as it’s only recently standardized in C… hence punting it to a new PR.
Since we aren't adding the decorator for deprecation warnings in this PR, is there anything else needed to close this?
I think that it is good to go.
Nothing from me.
Is config_get_int
and config_set_int
being used anywhere? I still think we can remove it without depreciating it.
Otherwise, I think we're in good shape here.
Let's keep config_get_int
and config_set_int
for now. If we remove them, we'd have to bump the library to version 2.0 for the ABI change. We need to get this out the door without delay. We can deal with the deprecated methods in the future.
See Issue #118
There has been desire to create a method by which users can configure where to store the key cache without setting the env var
$XDG_CACHE_HOME
or relying on the user to have a home dir. This is especially relevant in cases where the library is embedded in a daemon (e.g. XRootD or HTCondor) that may have a preferencial key cache location. This commit adds a familiarconfig_set_str
andconfig_get_str
API for passing a new cache home to the library through the use of thekeycache.cache_home
key.As with
config_set_int
andconfig_get_int
, these new functions can be easily updated in the future as the project develops and new configuration keys are desired. This would entail adding a new check for the desired key and the logic to handle any values associated with that key.Some comments/questions:
XDG_CACHE_HOME
. Currently, the implementation tries to create whatever directory is passed through the API if it doesn't exist, but this could break if the user has insufficient permissions to write to that location, or the path contains invalid chars (and probably some other things I'm not thinking of).XDG_CACHE_HOME
is set to a directory that doesn't already exist, the current code will fail if any parent dirs need to be created. The new config API attempts to create those parent dirs, so maybe a stub class to handle these types of things would be useful.