asu: revert sanitize #1554
Conversation
This reverts commit 1579236. Since the issue which originated that commit has been fixed[1] revert it to avoid other issues related to device identification when the devices share SUPPORTED_DEVICES strings[2]. [1]: openwrt/openwrt#21095 [2]: openwrt#1525 (comment)
This reverts commit 2e9edfd. Since the issue which originated that commit has been fixed[1] revert it to avoid other issues related to device identification when the devices share SUPPORTED_DEVICES strings[2]. [1]: openwrt/openwrt#21095 [2]: openwrt#1525 (comment)
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1554 +/- ##
===========================================
+ Coverage 80.75% 92.37% +11.61%
===========================================
Files 15 15
Lines 977 1429 +452
===========================================
+ Hits 789 1320 +531
+ Misses 188 109 -79 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@efahl what's your opinion here? |
|
Completely unneeded as the root cause has been fixed upstream in openwrt/openwrt#20632 (which also fixes firmware selector's metadata and re-enables sysupgrade's ability to reject bad images for that profile). Which reminds me I need to get @hauke to backport it to 24.10 (and 25.12 if it's not already there). |
|
Oh, and I should add that this would restore the bug that caused old LuCI app requests to fail occasionally. |
|
So I'll close this? |
|
Yes, unneeded. |
|
This has become just bigger that it really is... Can we restart here? Can we agree that replacing @aparcar the only feedback I got from you was this fully delegating, please, can I ask you for a real review? Should I apologize, should I introduce to myself before asking for review? It happens that we are in AI age and my slowly and carefully written, as I am trying to do my best, even for simple PR, are freely marked as IA slop and I am done? Please, forget the specific gl-mt2500 case. I am just trying to make clearer how ASU handle the device profile identification process which has become really confusing with the "sanitization" here and there. I have looked in all actors: the server and clients. A proposal of "cleaning"(no functional change) in owut efahl/owut#71 |
This reverts commit 1579236. After commit 2e9edfd ("build-request: sanitize profile before use") sanitized the incoming profile but not the lookup table, clients sending DTS compatibles got "profile not found". This fixed the mismatch by sanitizing both sides. Same issue: conflates board_name with profile name. Link: openwrt#1516 Link: openwrt#1554 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com>
This reverts commit 2e9edfd. The ',' -> '_' replacement conflates board_name with profile name. The fix was guided by owut's pre-translation behavior, which sends profile names directly, not by how other clients actually work. The trigger was a profile name using 'bananapi' while the DTS compatible uses 'bpi'; no string replacement could bridge that. The real fix was upstream, syncing profile names with DTS compatibles (openwrt/openwrt@d871e95e, openwrt/openwrt#21095). Link: openwrt#1511 Link: openwrt#1554 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com>
Since it seems there is no enough review time #1533 and the PR fixing the underlying issue which misleaded to the referenced revert commits has been merged:
maybe just reverting to an known working state have better luck.
This would allow to use firmware-selector for the gl-mt2500 MaxLinear PHY case even Owut or Luci ASU app with the workaround of
echo "glinet_gl-mt2500" > /tmp/sysinfo/board_namefor the MaxLinear users.The key here, IMHO, is that we have to make clearer the difference between "profile" and "DT compatible string" and for that I am thinking about some variable/messages renames could help also, but in order to prioritize, the code implied in the proposed reverts is the key and I think it goes in opposite direction.
#1525 (comment)
Right now we got this when generating the app.profiles[version][target]
Before commit "sanitize profile" 1579236
This do not implies nothing about which of PR fixing the gl-mt2500 issue in the openwrt repo will be finally merged: