Skip to content

cmake: drop the atomic-probe heuristic; find libatomic directly on 32-bit (#643) - #662

Merged
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/atomic-32bit-find-library
May 20, 2026
Merged

cmake: drop the atomic-probe heuristic; find libatomic directly on 32-bit (#643)#662
mrjimenez merged 1 commit into
amule-project:masterfrom
got3nks:fix/atomic-32bit-find-library

Conversation

@got3nks

@got3nks got3nks commented May 20, 2026

Copy link
Copy Markdown
Contributor

References #643. Follow-up to #648.

Bug

@barracuda156 reported on #643 that the libatomic-detection added in #648 still doesn't link on PPC32 MacPorts. The configure step reports:

-- Performing Test amule_HAVE_NATIVE_ATOMIC64 - Success

so cmake takes the "no -latomic needed" branch — and the actual amuled link then fails with the same __atomic_compare_exchange_8 / __atomic_load_8 / etc. symbols the original probe was supposed to detect.

Root cause

cmake/atomic.cmake calls check_cxx_source_compiles on a tiny snippet that creates a std::atomic<int64_t> and exercises store / load / fetch_add / compare_exchange_strong in a single main(). With everything visible to the compiler in one translation unit, GCC inlines the lock-free expansion and the probe links — no __atomic_*_8 references are emitted at all. The real codebase's atomics live in CDownloadBandwidthThrottler member functions called across translation-unit boundaries; the compiler can't inline those, and it does emit library-call references that need -latomic. Probe says "native works", binary says "no it doesn't".

Fix

Drop the probe entirely. On 32-bit targets the answer is unconditional: the CPU doesn't have a hardware 8-byte CAS, so libatomic is required. Just find_library(NAMES atomic) and link it. On 64-bit targets the path is a no-op. No check_cxx_source_compiles involved, no probe pathology possible.

cmake/atomic.cmake is deleted; the new logic lives at the top of the root CMakeLists.txt where the include used to be. The hard-fail message remains, with the same per-distro install hint the previous FATAL_ERROR carried — now triggered when find_library returns NOTFOUND on a 32-bit target rather than when the second probe fails to link.

Verification

Built clean on macOS arm64 (64-bit path, no-op as expected — no LIBATOMIC mentioned at configure or link). @barracuda156, would appreciate confirmation on PPC32 MacPorts once this lands; expectation is the configure log shows:

-- 32-bit target: linking libatomic for std::atomic<int64_t>

and the amuled link succeeds without the four __atomic_*_8 undefined-symbol errors.

Reported by @barracuda156.

PR amule-project#648's cmake/atomic.cmake uses check_cxx_source_compiles to decide
whether linking libatomic is required for std::atomic<int64_t>. The
probe runs a tiny main() that stores / loads / fetch_adds /
compare_exchanges a std::atomic<int64_t>, then links it. On 32-bit
targets the compiler can inline the entire lock-free expansion of
those operations end-to-end -- no __atomic_*_8 library-call symbols
are emitted, the link succeeds, and the probe reports 'native 64-bit
atomics work, no -latomic needed'.

The real codebase's atomics live in CDownloadBandwidthThrottler
member functions called across translation-unit boundaries. GCC
can't inline those, emits __atomic_compare_exchange_8 /
__atomic_load_8 / __atomic_store_8 / __atomic_fetch_add_8 references,
and the link fails with the original Undefined Symbols errors the
probe was supposed to detect. Reported by @barracuda156 on PPC32
MacPorts -- amule_HAVE_NATIVE_ATOMIC64 = Success at configure, link
failure at build.

The probe heuristic is fundamentally fragile (any in-TU inlining
hides the library-call references) so drop it. 32-bit CPUs lack a
hardware 8-byte CAS -- the answer is always 'libatomic required'.
Just look up the library with find_library(NAMES atomic) and link
it; 64-bit targets stay a no-op.

Move the new logic to the top of the root CMakeLists.txt where the
include statement used to be. cmake/atomic.cmake is deleted. The
hard-fail message preserves the per-distro install hint the
previous FATAL_ERROR carried.

Closes amule-project#643 properly.
@mrjimenez
mrjimenez merged commit 22ba4f2 into amule-project:master May 20, 2026
12 checks passed
@got3nks
got3nks deleted the fix/atomic-32bit-find-library branch May 22, 2026 13:50
mrjimenez pushed a commit to mrjimenez/amule that referenced this pull request Jul 28, 2026
…e-project#662)

The /eMule/CreateSparseFiles preference was EC-wired and settable from the Web UI but had no control in amule or amuleGUI, so hand-editing amule.conf was the only way to change it. Add a checkbox on the Files preferences page and move the pref from the untracked s_MiscList into the standard NewCfgItem/Cfg_Bool binding used by every sibling control. The setting only does real work when the core runs on Windows -- on POSIX both branches create the part file identically -- so the tooltip documents that, and the monolithic non-Windows build (where inertness is a compile-time certainty) hides the control after creation, keeping the binding intact so the value still round-trips through the config and EC. Closes amule-project#653.
ngosang pushed a commit to ngosang/amule that referenced this pull request Jul 28, 2026
…-project#663)

* feat(gui): remove the global Connect/Disconnect toolbar button

Closes amule-project#402. Per-network controls already exist (Kad pane's Start/Stop,
Servers pane's ED2K connect/disconnect), the connection state is
already shown in the status bar, and after discussing relocating it
into the Networks view the agreed call (got3nks + ngosang) was simply
to remove it -- mirrors what amule-project#585 already did on the Web UI side.

Removed the toolbar tool, its three skin icons (Toolbar_Connect/
Disconnect/Connecting) and their bitmap wiring, the button-update block
in ShowConnectionState(), and the two EnableTool(ID_BUTTONCONNECT, ...)
call sites.

CamuleDlg::OnBnConnect() itself stays -- CMuleTrayIcon::DoConnectDisconnect()
still calls it for the tray icon's own connect/disconnect action, which
is unaffected by this change.

ShowConnectionState()'s skinChanged parameter was only ever read by the
now-removed block, so it's gone too, along with the one call site that
passed true and the forceUpdate plumbing that reached it through
GuiEvents.cpp's ShowConnState() free function.

Left ID_BUTTONCONNECT and muuli_wdr.cpp's muleToolbar() (which still
calls it) alone -- that function predates Apply_Toolbar_Skin, is never
called anywhere, and touching pre-existing dead code is out of scope
for this fix.

Verified: built and ran both amule and amulegui cleanly, full unit
test suite passing (26/26). Confirmed via the po/ diff that "Connect"/
"Disconnect"/"Cancel" remain in the catalog (still used by other
per-network buttons) -- only their now-orphaned toolbar-specific
tooltip strings were dropped. Could not get a real on-screen visual
confirmation of the toolbar layout -- same macOS Accessibility
automation limitations as amule-project#180 got in the way of screenshotting the
actual app window.

* i18n: regenerate po/ catalogs after rebasing onto current master

Mechanical rebase to resolve conflicts against master's amule-project#642/amule-project#643 --
no source changes here. Resolved a real conflict in amuleDlg.cpp's
event table: amule-project#642 added the Alt+<letter> EVT_MENU block right where
this branch removed the EVT_TOOL(ID_BUTTONCONNECT, ...) line; kept
amule-project#642's block, dropped the connect-button line as intended.

Regenerated again via scripts/update-po.sh after later rebasing onto
master's amule-project#671 (Preferences sidebar) and amule-project#665/amule-project#662 (amuleapi
credentials, sparse part-file setting), which had added strings this
branch's catalogs didn't have yet.

* feat(gui): turn the per-network Disconnect buttons into Connect/Cancel/Disconnect toggles

Relocates the removed global toolbar button's functionality into the
ED2K (IDC_ED2KDISCONNECT) and Kad (ID_KADDISCONNECT) panes instead of
just dropping it, per got3nks's proposal on amule-project#663: each button now
mirrors ed2kState/kadState (off/connecting/connected) using the
existing connButImg() bitmaps, and can independently connect or
cancel/disconnect its own network -- something the old OR-toggled
global button never allowed. The ED2K pane also gains a Connect
button it never had (previously Disconnect-only).

CServerWnd::UpdateED2KConnectButton() / CKadDlg::UpdateConnectButton()
are called from CamuleDlg::ShowConnectionState() alongside the
existing UpdateED2KInfo()/UpdateKadInfo() calls, and once from each
pane's own ctor/Init() for the initial paint. OnBnClickedED2KDisconnect
and OnBnClickedDisconnectKad gained the missing "connect when off"
branch; OnBnConnect and the tray icon are untouched.

Verified interactively on Windows: both buttons show the correct
label/icon for their live state, ED2K disconnect/reconnect works from
its own button, and disabling ED2K in Preferences while Kad stays
connected confirms the two toggle independently.

po/ regenerated via scripts/update-po.sh to match the new button
labels ("Disconnect Kad" is now unused and moves to the obsolete
section; no new translatable strings).

* fix(gui): drop the per-network connect-button state cache

got3nks flagged two bugs in review (PR amule-project#663): the static cache's
early return also skipped Enable(), so the button could get stuck
disabled once IsReady()/network-pref state changed without the
Connect/Cancel/Disconnect state itself changing (master had two now-
deleted escape hatches -- the skinChanged force-path and an
unconditional EnableTool call -- that used to paper over this). Worse,
the cache was function-scope static, surviving widget recreation, so
a fresh pane's Init() call could early-return and leave the wxDesigner
default label ("Disconnect Kad") on screen.

Dropping the cache removes both: SetLabel/SetBitmap/Enable are cheap
on a plain wxButton (unlike the old toolbar tool, which had real
native-image-list reasons to memoize, see amuleDlg.cpp:1054-ish), and
master's own second call site already did an unconditional EnableTool
every tick without issue.

Second review round (macOS + Ubuntu ARM64, monolithic + amuleGUI)
confirmed the toggle works functionally but flagged three cosmetic
issues, all fixed here:

- No gap between the connect-button icon and its label. The obvious
  fix, wxButton::SetBitmapMargins(), is NOT portable (wxOSX overrides
  it, wxGTK inherits the base no-op) -- prefixing the label with a
  literal space outside the _() call behaves identically on both and
  introduces no new msgid.
- The Kad button was stretched full-width by its sizer's .Expand()
  flag, while the ED2K one sizes naturally; GTK and macOS then filled
  the slack differently (centered vs. icon-left). Dropped .Expand()
  so the two buttons size the same way -- trades away lining up with
  the "Bootstrap from known clients" button above it, but the two
  connect buttons reading as the same control matters more.
- CServerWnd::UpdateED2KConnectButton() and CKadDlg::UpdateConnectButton()
  were near-identical (same 3-state enum, same switch, same three
  bitmaps). Factored into a shared SetConnectButtonState(button, state,
  enabled) helper in muuli_wdr.cpp/.h, next to connButImg() which it
  reuses -- so the icon-spacing fix above lives in one place instead
  of two.

Verified interactively on Windows: re-tested full disconnect/reconnect
cycle on both panes, toggling ED2K off and back on via Preferences
(existing behavior: needs an app restart to re-enable) with correct
state on the fresh post-restart paint, and confirmed by screenshot
that both buttons now show a visible icon/label gap and size the same
way (Kad no longer stretched). Also confirmed amulegui (CLIENT_GUI)
builds clean with these changes; did not perform full interactive
remote-daemon testing.

* fix(gui): first-paint layout bug + connect-button placement, per review

Two more issues from got3nks's Linux/GTK pass on amule-project#663.

1. Bug: the ED2K connect button painted with the icon overlapping the
   label on first show (Kad was fine, only by luck of timing).
   CServerWnd::UpdateED2KConnectButton() runs after sizer->Show(this,
   TRUE) in the ctor, so SetBitmap() grows the button's best size
   after the row was already measured against the text-only label,
   and nothing re-lays it out until a window resize. Fixed by calling
   Layout() on the button's parent at the end of the shared
   SetConnectButtonState() helper -- covers both panes, and stops Kad
   from relying on being laid out late (its notebook page is only
   measured once shown, after Init() already set its bitmap).

2. Layout: moved both connect/disconnect toggles to lead their pane
   instead of sitting among secondary controls, per got3nks's request:
   - ED2K (serverListDlgUp): toggle now on its own top row, right-
     aligned via a stretch spacer, directly above the server list.
     The "Add server manually" form (name/IP/port/Add) moved below
     the table instead of sharing a row with the toggle; the vertical
     separator that used to sit between them is gone (no longer
     needed once they're not adjacent).
   - Kad (KadDlg): toggle moved out of the "Bootstrap" static box
     (where it sat under "Bootstrap from known clients") to its own
     top row above the box, right-aligned the same way as ED2K's.
     item0 changed from a 1x1 FlexGridSizer (which only ever held the
     two-column area) to a plain vertical wxBoxSizer so it can hold
     the new top row plus the existing two-column layout.

Both panes now read "primary control on top, secondary/manual
controls below" -- matching shape on both tabs.

Verified interactively on Windows: screenshotted both panes, confirmed
no icon/label overlap on first paint (no window resize needed), and
re-ran the disconnect/reconnect toggle cycle on both after the layout
change.
mrjimenez pushed a commit to mrjimenez/amule that referenced this pull request Jul 30, 2026
…able, Web UI label CI gate (amule-project#655) (amule-project#696)

* refactor(amuleapi): make /preferences field names self-explanatory (amule-project#655)

Rename 45 of the 119 fields GET/PATCH /api/v0/preferences exposes so each
one states what it does without a lookup table, and nest the two unrelated
remote-control subsystems instead of prefixing every field. amuleapi has
not shipped and the protocol stays at v0, so the old names are simply
removed rather than dual-emitted.

The EC protocol is untouched: every tag number, presence-vs-value encoding
and thePrefs:: accessor stays as-is, so amulegui / amulecmd / amuleweb
remain compatible. All translation happens at the webapi boundary, the
same way extended_udp_port_enabled already inverts EC_TAG_CONN_UDP_DISABLE.

Highlights, by the class of problem each fixes:

- Misleading names: security.obfuscation_supported -> obfuscation_enabled
  (it is a writable preference, while _supported / _available elsewhere
  means a read-only build capability); message_filter.friends / .secure /
  .all -> accept_from_friends_only / accept_from_known_clients_only /
  filter_all_messages; directories.exclude_regex ->
  exclude_patterns_use_regex (a modifier on exclude_patterns, not a second
  list); files.preview_prio -> prioritize_first_last_chunks.
- Missing units: core_tweaks.srv_keepalive_timeout ->
  server_keepalive_timeout_ms (its two siblings already said _ms),
  connection.slot_allocation -> upload_slot_kbps, filebuffer ->
  file_buffer_bytes, online_signature.update_frequency ->
  update_frequency_seconds, webserver refresh -> refresh_seconds.
- Magic numbers: connection.proxy_type and security.can_see_shares (now
  shared_files_visibility) become enum strings. A new PrefTakeEnum helper
  maps them to the wire ints and also replaces the hand-rolled inline
  mapping ip2country.source was using.
- Structure: remote_controls.{webserver,amuleapi}.{...} replaces the nine
  webserver_* / amuleapi_* prefixes. Both sub-objects still pack into the
  single EC_TAG_PREFS_REMOTECTRL group, and passwords stay write-only.
- Abbreviations spelled out: dl / ul / prio / cat / srv / autoconn.

files.create_normal is deliberately left alone. It is the one field whose
positive name requires inverting its meaning, so it gets its own change.

The Web UI moves with the API in the same commit: field keys, the derived
prefs_field_* dictionary keys in both locales, and dotted category paths
for the nested objects. Two labels copied verbatim from the desktop are
reworded ("Slot Allocation" -> "Upload speed per slot (KB/s)"), and the
keepalive field now scales to minutes like the desktop slider instead of
showing a bare millisecond count.

check-i18n.mjs gains a check that every preferences field resolves to a
real label, and is wired into the i18n workflow. These label keys are
derived from the field names, so before this a renamed field with no
dictionary entry would have rendered the raw key with nothing failing.

Verified: full macOS build clean, 28/28 ctest green, curl phases 05 (73)
and 15 (117 incl. new nested remote_controls and enum coverage) pass
against a live daemon. Documented and emitted key sets diffed to zero.

* refactor(amuleapi): drive /preferences from one declarative field table (amule-project#655)

The 119 fields on /api/v0/preferences were described three times, in three
different idioms: a hand-written EC decode per category in Refresher, a
w.Key/w.Value pair per field in the GET emitter, and a PATCH applier that
had grown two parallel helper families -- 87 call sites on the generic
PrefTake* helpers plus 22 more on connection-local lambdas doing the same
job with their own error strings and a different return convention. A field
touched in two of the three and missed in the third compiled cleanly.

Replace all three with a walk over one table (PrefsSchema.{h,cpp}). Each row
names a field's JSON category and key, its EC tag, its type, how EC encodes
it, and whether it is readable, writable or neither. Adding a preference is
one row; renaming one is one token.

What used to be special-case code is now a column:

- `invert` -- extended_udp_port_enabled, whose EC tag (EC_TAG_CONN_UDP_DISABLE)
  is negatively named. This is also what makes the deferred create_normal ->
  create_sparse_files flip a one-value change instead of two structurally
  different edits on the read and write paths.
- `enc` -- presence-tag vs value-tag booleans, which the core serializer mixes
  (57 presence, 9 value) and which every reader previously had to know.
- `access` -- read-only capabilities and live status, write-only passwords and
  the ip2country trigger, and the amuleapi passwords that belong to
  /auth/passwords and are rejected here.
- `gated_by` -- the 409 when files.mmap_enabled is set against a core built
  without mmap.
- `read_group` -- connection.upnp_available, the one field whose EC tag lives
  in a different group ([General]) than its JSON category implies.

Rows carry the address of their backing member, and the PREF_* macros
static_assert that the declared PrefType matches the member's real C++ type,
so a mis-typed row fails the build rather than misbehaving at runtime.

One field genuinely cannot be described: remote_controls.webserver
.guest_enabled and .guest_password share a single EC tag, which carries the
enable bool as its value and the password hash as a child. There is no 1:1
field-to-tag mapping to write down, so its PATCH packing stays hand-written
and the schema marks it Bespoke. Its GET emission is still table-driven.

Equivalence was checked against a captured GET /preferences from the previous
build: the EC decode and the GET emitter each produce byte-identical output,
and the final key set matches at 119/119. PATCH is covered by curl phase 15
(117/117, unchanged) plus direct checks that every type, the inverted field,
the nested categories and each error path still behave. Error bodies are now
uniform ("<key> must be a bool") where they used to be per-category
("connection field must be a bool"); no test or doc asserted the old wording.

Two new tests pin the table's own invariants: no duplicate fields, every
category resolving to an EC group, members present exactly for the rows that
round-trip a value, enum rows carrying a name table, capability gates naming a
real sibling bool, and the emitted count staying at the documented 119. A
second test asserts the three irregularities above stay at one field each, so
a second one appearing is caught rather than copied.

Net: -1437 / +842 lines, with the prefs handling in Api.cpp down 912 lines and
Refresher.cpp down 212.

* feat(amuleapi)!: state the sparse-file preference positively (amule-project#655)

files.create_normal becomes files.create_sparse_files, and its meaning
inverts: create_sparse_files == !create_normal. This is the one field in the
payload whose positive name could not be reached by renaming alone, so the
issue asked for it on its own commit.

Every layer except EC already spoke in terms of "sparse". The config key is
/eMule/CreateSparseFiles, the core stores s_createFilesSparse (default on),
PartFile.cpp reads CreateFilesSparse(), and the desktop checkbox added in
amule-project#662 is labelled "Create new files as sparse files". Only the EC tag is named
for the negative case -- EC_TAG_FILES_CREATE_NORMAL is an empty tag present
only when sparse is off -- and amuleapi was the one consumer that took that
name and published it outward, leaving the Web UI saying "non-sparse" where
the desktop says "sparse" for the same setting.

EC is untouched: same tag number, same presence semantics, same
thePrefs::CreateFilesNormal(). The negation is undone at the webapi boundary
instead, exactly as extended_udp_port_enabled already does for
EC_TAG_CONN_UDP_DISABLE. Thanks to the schema table this is one column:

    PREF_BOOL("files", "create_sparse_files", EC_TAG_FILES_CREATE_NORMAL,
              PrefEnc::Presence, /*invert=*/true, ...)

The snapshot default flips to true to match the core's own default, and the
Web UI label moves to the desktop's positive wording, which also flips the
checkbox's rendered state for an unchanged setting.

Verified end to end against a daemon's own config file rather than just the
API's echo, since a polarity change is precisely where a self-consistent API
can still be wrong:

  CreateSparseFiles=1  -> GET create_sparse_files: true
  PATCH false          -> CreateSparseFiles=0 in amule.conf
  CreateSparseFiles=0  -> GET create_sparse_files: false
  PATCH true           -> CreateSparseFiles=1 in amule.conf

The schema-invariant test caught this change on its own: it enumerates which
fields may invert, so adding a second one failed until the list was updated
deliberately. That is the check working, and the list now names both tags and
why each is negative.

Note for clients: this is the one field where renaming the key without
flipping the value yields the opposite behaviour. Nothing ships with the old
name -- amuleapi is unreleased and the protocol stays at v0 -- so no
compatibility shim is carried.

* fix(amuleapi): keep the enum index unsigned in the prefs decoder

CECTag::GetInt() returns uint64_t, so copying it into an std::int64_t
narrows, which bugprone-narrowing-conversions flags on a new line. Keep the
index unsigned: the >= 0 half of the range check was dead weight anyway, and
the remaining bound is the only one that matters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants