(FACT-3423) sys-filesystem updates #2604
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mhashizume
force-pushed
the
FACT-3423/main/sys-filesystem-fix
branch
2 times, most recently
from
2f63622 to
3956912
Compare
joshcooper
reviewed
Aug 29, 2023
mhashizume
force-pushed
the
FACT-3423/main/sys-filesystem-fix
branch
6 times, most recently
from
22e1b2a to
2908ce3
Compare
In 19c2bb1, we moved sys-filesystem from a development to runtime dependency. This was done because we mistakenly believed that a LoadError introduced in Facter 4.4.2 was caused because sys-filesystem was not a runtime dependency. The LoadError was actually caused by another change: updating the base resolver to log an error instead of a debug message on LoadError (50bbef9). The team that originally wrote Facter 4 intended for sys-filesystem to be a development dependency due to its dependency on ffi. ffi contains native extensions and would mean that `gem install facter` would require a compiler. This commit moves sys-filesystem back from a runtime to development dependency and leaves a comment in the gemspec explaining the reasoning for its placement.
In 50bbef9, we changed the base resolver's behavior to cause LoadErrors to log an error by default. Previously, we had only logged a debug message. This commit updates several mountpoint resolvers to rescue LoadError when calling the `read_mountpoint_stats` method, which requires the sys-filesystem gem. sys-filesystem is only listed as a development dependency (see dbc5a7c) and may not be present at runtime.
The ffi gem is a dependency for the FFI module in Facter. However, the dependency on the ffi gem is only met as a transitive dependency through sys-filesystem. This commit adds the ffi gem as a development dependency to the gemspec. The ffi gem is being added as a development dependency, as opposed to a runtime dependency, because it contains native extensions. The original intent was to allow users to install Facter without a compiler to be as portable as possible, and native extensions would prevent that from happening.
mhashizume
force-pushed
the
FACT-3423/main/sys-filesystem-fix
branch
4 times, most recently
from
d1c1257 to
0bddf6b
Compare
The ffi gem is required in many parts of Facter, but is not guaranteed to be present because it is only a development dependency, not a runtime dependency. This commit updates modules that require ffi to add exception handling for when ffi has not been installed and a LoadError is thrown.
mhashizume
force-pushed
the
FACT-3423/main/sys-filesystem-fix
branch
from
0bddf6b to
f912088
Compare
tvpartytonight
approved these changes
Sep 8, 2023
There was a problem hiding this comment.
I'm not sure if there is a better pattern than the implementing a LoadError rescue for each of these locations, seems like a whack-a-mole issue that could end up being problematic later on, but I think it works for now to remove the unwanted transitive dependency.
joshcooper
approved these changes
Sep 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR: