fix(amuleweb,EC): remote.conf round-trip + Windows webserver template path - #822
Conversation
…freshTime amuleweb writes its remote.conf settings under two different section names: [Webserver] (lowercase 's') for most keys, but [WebServer] (capital S) for UPnPTCPPort — an old typo, with the result that the two UPnP-related keys (UPnPWebServerEnabled and UPnPTCPPort) end up in different sections despite belonging together. amule.conf already uses [WebServer] consistently, so that's the canonical spelling. Migration: - LoadConfigFile reads each key from [WebServer] first; if missing, falls back to the legacy [Webserver]. Existing installs upgrade without losing settings. - SaveConfigFile writes only under [WebServer] and deletes the legacy [Webserver] group, so after one save round-trip the duplicate section disappears. While here: PageRefreshTime was read in LoadConfigFile but never written in SaveConfigFile — any value the user set in remote.conf silently reset to the 120s default on the next launch. Persist it now so the field actually round-trips. Closes amule-project#818.
CaMuleExternalConnector::LoadConfigFile reads /EC/ZLIB but the matching SaveConfigFile never wrote it — toggling --disable-zlib / --enable-zlib at the command line silently failed to persist, and any non-default value the user set in the config file reset to 1 (enabled) on the next save round-trip. Add the missing Write next to the other /EC/* keys. Affects amulecmd, amuleweb, and amulegui (all subclass CaMuleExternalConnector). Part of amule-project#817.
got3nks
changed the title
CaMuleExternalConnector::LoadConfigFile() read /EC/Host with an empty default and relied on a runtime fallback in OnCmdLineParsed to set "localhost" if neither --host nor the config file provided a value. amulegui (amule-remote-gui.cpp:84) already reads the same key with "localhost" as the default directly. The result was an empty "Host=" line written to remote.conf by amulecmd/amuleweb on first save, diverging from what amulegui would write to the same file. Use "localhost" as the default here too so all three tools share the same canonical default. The OnCmdLineParsed runtime fallback stays as a safety net for the "no config file at all" path. Part of amule-project#821.
got3nks
changed the title
|
@ngosang — packaging dispatched, all platforms, branch Once green, scroll to "Artifacts" at the bottom (GitHub login needed) and extract any platform's binaries. Manual test plan items the PR body lists:
|
|
Something is broken... Before this PR: After this PR with the same command: amuleweb is not working because the admin pass is empty |
wxFileConfig group names are case-insensitive, so DeleteGroup("/Webserver")
matched the [WebServer] section the preceding writes just created and
wiped every key in it. Reported by @ngosang on amule-project#822: running
`amuleweb --write-config -h localhost -p 4712 -P 12345678 -A 12345678`
produced a remote.conf with no [WebServer] section at all, breaking
the tool in any subsequent run.
Verified on amule-dev-vm:
- Before fix: only [EC] section in the output file
- After fix: [WebServer] section present with all keys
The case-insensitivity also makes the HasEntry/fallback in
LoadConfigFile dead code on every wxWidgets backend in current
practice — kept as a defensive no-op in case a future port breaks
the assumption.
Part of amule-project#822.
…eDir GetTemplateDir's third fallback uses wxStandardPaths::GetResourcesDir() to find webserver templates, with a Linux-specific BeforeLast+JoinPaths dance that normalizes the path against the FHS share/amule layout. The Windows arm of the same #if (amule-project#783's earlier adjustment in Preferences.cpp::Cfg_Path::TransferToWindow) was missing here, so on the Windows portable / NSIS-installed layout — exe in bin\, data in ..\share\amule\ — wxStandardPaths returned the bin directory and the template search fell off the end at "FATAL ERROR: Cannot find template: default". Mirror the same JoinPaths(..,"..","share","amule") adjustment as Preferences.cpp uses for the skins case. Closes amule-project#828.
got3nks
changed the title
|
Confirmed the regression —
Dropped in commit Full Packaging run for the updated branch (so you can grab a built Sorry about the round-trip! |
|
@ngosang if you can confirm on Windows the |
|
@mrjimenez ready to merge — final test-plan item ticked. Manually verified the Windows portable template-path fix on an ARM64 Windows 11 VM: |
|
@got3nks Don't rush to merge this PR. We need to test it thoroughly. I don't think aMuleWeb has ever worked on Windows.
This error is fixed with your latest changes, but I found more problems. Starting amuleweb from the consoleWindows 11
There are 2 problems in the file:
I fixed the second problem changing Starting amuleweb from amule
If I run |
….0.1 Two follow-ups on @ngosang's regression report on the amule-project#822 packaging build: A. The output `remote.conf` was still emitting `[Webserver]` (lowercase `s`) for the webserver section even though every code path writes `/WebServer/<key>` (capital `S`). Root cause was the lambda fallback I added in CamulewebApp::LoadConfigFile: on a fresh file, HasEntry("/WebServer/Port") returns false, so the fallback path `Read("/Webserver/Port", def)` ran. That lowercase Read registered the section's case in wxFileConfig's internal map; subsequent `/WebServer/<key>` writes then merged case-insensitively into the already-locked-lowercase group, and the INI file emitted `[Webserver]`. Drop the fallback entirely. wxFileConfig's case-insensitive group matching means `Read("/WebServer/<key>", ...)` already finds entries that older builds stored under `/Webserver/<key>`; no separate legacy path is needed. The section in the output file now consistently emits as `[WebServer]`, matching the case `amule.conf` already uses. B. `Host=localhost` doesn't reliably work on Windows — @ngosang reported `Connection Failed. Unable to connect to localhost:4712` on a fresh `remote.conf`. The cause is the usual Windows-side IPv4/IPv6 stack ordering / hosts-file shape that makes `localhost` name resolution flaky. `127.0.0.1` is portable across every supported OS. Default `/EC/Host` to `"127.0.0.1"` in: - CaMuleExternalConnector::LoadConfigFile (the config-file default) - CaMuleExternalConnector::OnCmdLineParsed (the runtime fallback when neither --host nor the config-file value is set) - CaMuleExternalConnector::LoadAmuleConfig (the --amule-config-file path used when amuleweb is launched by amule) - amule-remote-gui.cpp:84 (amulegui's own /EC/Host default) The user-visible help string `"Host where aMule is running. (default: localhost)"` deliberately keeps the word "localhost" — it's semantic documentation and changing it would invalidate every translation. The actual default in code is 127.0.0.1. Part of amule-project#818, amule-project#821, amule-project#822 (follow-ups within amule-project#822).
|
Both fixes pushed in A. Worth noting for reviewers: wxFileConfig group names are case-insensitive, so reading B. Updated packaging for testing: https://github.com/got3nks/amule/actions/runs/26882511922 For the third issue (amuleweb exits silently when launched from amule), the clean status-0 exit with no log line suggests something bails without printing. Could you re-test with |
wxFileConfig group names are case-insensitive, so DeleteGroup("/Webserver")
matched the [WebServer] section the preceding writes just created and
wiped every key in it. Reported by @ngosang on #822: running
`amuleweb --write-config -h localhost -p 4712 -P 12345678 -A 12345678`
produced a remote.conf with no [WebServer] section at all, breaking
the tool in any subsequent run.
Verified on amule-dev-vm:
- Before fix: only [EC] section in the output file
- After fix: [WebServer] section present with all keys
The case-insensitivity also makes the HasEntry/fallback in
LoadConfigFile dead code on every wxWidgets backend in current
practice — kept as a defensive no-op in case a future port breaks
the assumption.
Part of #822.
|
I tested the last commit and all problems are fixed. |
|
Most likely yes - PR is already merged. |
The --host default for amulecmd/amuleweb was changed to 127.0.0.1 (over "localhost") to avoid intermittent loopback resolution failures on Windows (#821, #822), but the docs, the --help string, the amulegui host field default, and the translation catalogs still said "localhost". Align all of them with the actual code default: - src/ExternalConnector.cpp: --help text - src/muuli_wdr.cpp: ID_REMOTE_HOST initial value - docs/man/*.1.in man pages + docs/man/po catalogs - po/ application catalogs (msgid + msgstr) No functional change; the runtime default (127.0.0.1) is untouched.
The Search tab's Name-field history held 30 entries, eMule's CCustomAutoComplete default, adopted in the amule-project#643 review rather than chosen for aMule. A history is only useful as far back as it reaches, and 30 queries is a short reach for anyone who searches often; clearing it has been a deliberate, confirmed action since amule-project#754, so there is less cost to keeping more of it around. Raised to 100 on request (amule-project#755). Eviction is LRU and stays that way: ApplySearchHistoryEntry moves a searched term to the front and drops any earlier case-insensitive copy, so what falls off the tail is the least recently searched rather than the least recently added. Terms that get reused stay near the front whatever the cap is, which is what makes the extra slots worth having -- they go to the long tail instead of hoarding stale one-offs. The constant moves from an anonymous namespace in SearchDlg.cpp to SearchHistory.h, beside the function that consumes it. That closes a gap in the tests: the one that claimed to lock "the constant CSearchDlg actually passes in" was hardcoding its own copy of the number, so it would have gone on passing while the GUI used something else entirely. It now reads the same symbol the GUI does. A longer history could have worsened the other half of amule-project#755 -- the dropdown overlapping other UI -- so the list was checked with a full 100 entries on Windows 11, Ubuntu and macOS first. None of the three runs it off the screen. Bounding the visible entries is a separate matter and not addressed here: wxWidgets exposes no dropdown-height control on plain wxComboBox (SetPopupMaxHeight is wxComboCtrl/wxOwnerDrawnComboBox only), so it needs a different widget rather than a setting.
3 participants
Summary
Closes #818, #817, #821, #828 — four related amuleweb bugs touching the same code paths:
amulewebwroteremote.confunder two inconsistent section names (Inconsistent section capitalization for web server UPnP keys:[Webserver]vs[WebServer]inremote.conf#818):[Webserver](lowercase s) for most keys,[WebServer](capital S) only forUPnPTCPPort. Cosmetic — wxFileConfig treats both as the same group, but the C++ code disagreed with itself.remote.confsettings are read but never written back (ZLIB,PageRefreshTime) #817 + drive-by):PageRefreshTime(amuleweb) and/EC/ZLIB(sharedCaMuleExternalConnector). Any value the user set in the config silently reset to the default on each save round-trip./EC/Hosthad different defaults across tools (Inconsistent default for/EC/Hostinremote.conf: empty (amulecmd/amuleweb) vslocalhost(amulegui) #821):amulecmd/amulewebdefaulted to empty (relying on a runtime fallback tolocalhost);amuleguidefaulted tolocalhostdirectly. Result was inconsistent writes to the sharedremote.conf.amulewebon Windows portable / NSIS-installed builds couldn't find webserver templates (Error in amuleweb: Cannot find template: default #828): the search path's third fallback usedwxStandardPaths::GetResourcesDir()without the Windows portable layout adjustment, so the search hitbin\webserverinstead of..\share\amule\webserverand failed withFATAL ERROR: Cannot find template: default. Same root cause as aMule skins are not shown in Windows #783's skins fix.Fixes
Section consistency (
src/webserver/src/WebInterface.cpp):LoadConfigFilereads each key from[WebServer]first, falling back to legacy[Webserver]. Defensive against future wx ports where the case-insensitive group assumption changes.SaveConfigFilewrites only[WebServer].Missing writes:
WebInterface.cpp::SaveConfigFilenow writesPageRefreshTime.ExternalConnector.cpp::SaveConfigFilenow writes/EC/ZLIBnext to the other/EC/*keys.Default alignment:
ExternalConnector.cpp::LoadConfigFiledefaults/EC/Hostto"localhost", matchingamule-remote-gui.cpp:84. The runtime fallback inOnCmdLineParsedstays as a safety net.Windows portable templates (
WebInterface.cpp::GetTemplateDir):JoinPaths(.., "..", "share", "amule")adjustment on Windows, mirroring the same fixPreferences.cppalready applies for skins per aMule skins are not shown in Windows #783.Regression caught during review
An earlier revision of this PR called
m_configFile->DeleteGroup("/Webserver")after writing the canonical[WebServer]keys, intending to migrate the section's case in the INI file. Confirmed via--write-configonamule-dev-vmthat wxFileConfig group names are case-insensitive —DeleteGroup("/Webserver")matched the just-written[WebServer]section and wiped every key in it. Dropped the DeleteGroup; the section's case is now whatever wxFileConfig already settled it as (functionally identical, since lookups are case-insensitive).Thanks @ngosang for catching it on #822.
Verify
cmake --build build --target amuleweb) green.--write-configregression reproduced and fix verified onamule-dev-vm:[WebServer]section is no longer wiped from the output file.remote.confcontaining[Webserver]entries from a prior release, runningamulewebonce and quitting still produces a working config (no key loss).PageRefreshTimein[WebServer]survives a restart (previously reset to 120)./EC/ZLIB=0inremote.confsurvives anamulecmd/amulewebquit-and-save (previously reset to 1).remote.confwritten byamulecmd/amulewebcontainsHost=localhostinstead ofHost=.amuleweb --amule-config-file=<path-to-amule.conf>reaches the EC password prompt without the "Cannot find template: default" fatal — confirms templates are now resolved via the portable layout's..\share\amule\webserver\path. Verified on an ARM64 Windows 11 VM.