Add Support for ENux#2391
Conversation
Up to standards ✅🟢 Issues
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR introduces support for ENux, but the current implementation of the detectBedrock function contains a logic error that breaks the fallback mechanism. Specifically, if the ENux stratum directory is found but the os-release file within it is missing or unparseable, the function returns false immediately instead of attempting to detect the standard Bedrock stratum.
Additionally, the use of FF_COLOR_FG_BLACK for branding colors poses a significant accessibility risk. On the majority of Linux terminals, which use a dark or black background, the fetch output will be invisible. While this may align with the distribution's branding, it severely impacts the usability of the tool. There are also several code style deviations (Allman vs K&R braces) that should be addressed for codebase consistency.
About this PR
- The decision to use
FF_COLOR_FG_BLACKfor the logo, keys, and title makes the tool's output functionally invisible on the most common terminal configuration (dark backgrounds). Consider using a more accessible alternative likeFF_COLOR_FG_LIGHT_BLACKor providing a toggle if this branding is mandatory. - No unit or integration tests were included to verify the new detection logic for Bedrock strata, increasing the risk of regressions in OS identification.
Test suggestions
- Verify that 'detectBedrock' correctly identifies ENux when the '/bedrock/strata/enux' directory exists.
- Verify that 'detectBedrock' falls back to the default 'bedrock' stratum when the 'enux' stratum is missing.
- Verify that OS detection is skipped when 'BEDROCK_RESTRICT' is set to '1'.
- Verify that the 'enux' logo is automatically selected when the OS ID is 'enux'.
- Verify that the 'enux' logo, keys, and title are rendered using 'FF_COLOR_FG_BLACK'.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that 'detectBedrock' correctly identifies ENux when the '/bedrock/strata/enux' directory exists.
2. Verify that 'detectBedrock' falls back to the default 'bedrock' stratum when the 'enux' stratum is missing.
3. Verify that OS detection is skipped when 'BEDROCK_RESTRICT' is set to '1'.
4. Verify that the 'enux' logo is automatically selected when the OS ID is 'enux'.
5. Verify that the 'enux' logo, keys, and title are rendered using 'FF_COLOR_FG_BLACK'.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| if (ffPathExists( | ||
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux", | ||
| FF_PATHTYPE_DIRECTORY)) | ||
| { | ||
| return parseOsRelease( | ||
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux/etc/os-release", | ||
| os); | ||
| } |
There was a problem hiding this comment.
🟡 MEDIUM RISK
If the 'enux' directory exists but the 'os-release' file within it cannot be parsed, the function returns false without attempting to detect the base Bedrock system. This breaks the fallback mechanism. The detection should only return if it successfully finds OS information.
Try running the following prompt in your coding agent:
Refactor the
detectBedrockfunction insrc/detection/os/os_linux.cto attempt parsing theenuxstratum'sos-releasefirst, and only return if it succeeds; otherwise, fall back to parsing the default Bedrock stratum'sos-release.
| .colors = { | ||
| FF_COLOR_FG_BLACK, | ||
| }, | ||
| .colorKeys = FF_COLOR_FG_BLACK, | ||
| .colorTitle = FF_COLOR_FG_BLACK, |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Using FF_COLOR_FG_BLACK for the logo, keys, and title will make the output invisible on dark-themed terminals. While this matches ENux branding, it is better to use FF_COLOR_FG_DEFAULT or FF_COLOR_FG_LIGHT_BLACK to ensure accessibility for the majority of users.
| static bool detectBedrock(FFOSResult* os) | ||
| { | ||
| const char* bedrockRestrict = getenv("BEDROCK_RESTRICT"); | ||
| if (bedrockRestrict && bedrockRestrict[0] == '1') { | ||
| if (bedrockRestrict && bedrockRestrict[0] == '1') | ||
| return false; | ||
|
|
||
| if (ffPathExists( | ||
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux", | ||
| FF_PATHTYPE_DIRECTORY)) | ||
| { | ||
| return parseOsRelease( | ||
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux/etc/os-release", | ||
| os); | ||
| } | ||
| return parseOsRelease(FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/bedrock/etc/os-release", os); | ||
|
|
||
| return parseOsRelease( | ||
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/bedrock/etc/os-release", | ||
| os); | ||
| } |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The function deviates from the project's preference for K&R style braces and mandatory braces for single-line if statements.
| static bool detectBedrock(FFOSResult* os) | |
| { | |
| const char* bedrockRestrict = getenv("BEDROCK_RESTRICT"); | |
| if (bedrockRestrict && bedrockRestrict[0] == '1') { | |
| if (bedrockRestrict && bedrockRestrict[0] == '1') | |
| return false; | |
| if (ffPathExists( | |
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux", | |
| FF_PATHTYPE_DIRECTORY)) | |
| { | |
| return parseOsRelease( | |
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux/etc/os-release", | |
| os); | |
| } | |
| return parseOsRelease(FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/bedrock/etc/os-release", os); | |
| return parseOsRelease( | |
| FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/bedrock/etc/os-release", | |
| os); | |
| } | |
| static bool detectBedrock(FFOSResult* os) { | |
| const char* bedrockRestrict = getenv("BEDROCK_RESTRICT"); | |
| if (bedrockRestrict && bedrockRestrict[0] == '1') { | |
| return false; | |
| } | |
| if (ffPathExists(FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux", FF_PATHTYPE_DIRECTORY)) { | |
| if (parseOsRelease(FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/enux/etc/os-release", os)) { | |
| return true; | |
| } | |
| } | |
| return parseOsRelease(FASTFETCH_TARGET_DIR_ROOT "/bedrock/strata/bedrock/etc/os-release", os); | |
| } |
1 participant
Add Support for ENux
What this does
src/logo/ascii/e/enux.txt) and registers itin
src/logo/ascii/e.incunder the nameenux. The logo, module keys, and titleall render in black (
FF_COLOR_FG_BLACK), matching ENux's all-black branding.ID, so no extra wiring is needed: when/etc/os-releasereportsID=enux, fastfetch picks this logo automatically.How ENux is detected
ENux ships with Bedrock Linux pre-integrated, so the OS-release lookup needed a patch
in
detectBedrock()(src/detection/os/os_linux.c). On a Bedrock system the defaultos-releasecomes from thebedrockstratum, which would make fastfetch report"Bedrock" instead of the actual distro. The patched logic checks for the
enuxstratumfirst and reads its
os-releasewhen present:This makes fastfetch report ENux (
ID=enux) on a Bedrock-based ENux install, which inturn triggers the ENux logo.