Repository navigation
Conversation
Introduced in 530a468 as an experiment to reduce fork memory. Measured at 50s of GC pauses in a 98s test run vs 0.14s with automatic GC only. Automatic GC still runs; removing this does not disable collection.
Drop the bash -l -c wrapper added in 44d1d93. The wrapper added ~50ms per launch for shell parsing, broke argument fidelity (naive space escaping), and re-sourced login profiles over the inherited environment instead of carrying it faithfully. ProcessBuilder children already inherit the JVM's full environment, user, and working directory. All in-tree callers pass plain argv; shell semantics for string commands are no longer supported on Linux.
With direct argv, a missing or non-executable command now throws from
runProcess(), as its javadoc always documented ('If there is any
problem with the process, throws an exception') - the removed shell
wrapper used to mask launch failures as exit code 127. Handle the
launch failure where graceful failure is the intent:
- SpecsGraphviz: dot being absent is a normal condition, so checkDot()
returns false and renderDot() logs instead of crashing.
- ProcessExecution: the jobs framework is built on return codes with
no exception handling, so run() reports the failed launch as
exit code 127 instead of propagating.
This also restores SpecsSystem.isCommandAvailable()'s intended
behavior: it detects missing commands by catching the launch
exception, which the wrapper's masking previously defeated.
…tedyet panels, legacy SetupData path)
- SpecsIo.createTempDirectory(prefix): unique directory allocated by the OS under the user temp folder, registered for deletion on exit as a backstop, so callers no longer hand-roll UUID/timestamp names - SpecsIo.getOsCacheFolder(): user-level cache folder following OS conventions (XDG_CACHE_HOME, ~/Library/Caches, %LOCALAPPDATA%) - SpecsSystem.isMac()
Build key maps before publishing definitions and atomically cache key indexes. A concurrent reader previously observed an incomplete map, allowing setRaw to silently discard AST fields.
Retain copied default suppliers in generic keys, add explicit temporary-directory cleanup, and select absolute Linux cache locations. Remove the stale GuiHelper composite include and add focused default and cleanup regression coverage.
Port regression coverage for parallel first readers, independent data-store creation, subclass-safe construction, immutable key maps, and section order. Keep the existing store implementation and its Protobuf optimizations. Focused AStoreDefinition and existing StoreDefinition concurrency suites passed offline; aggregate coverage was excluded for this targeted run.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76fe6b1586
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for (StoreSection section : this.sections) { | ||
| for (DataKey<?> key : section.getKeys()) { |
There was a problem hiding this comment.
When an AStoreDefinition subclass overrides getKeys() to add or transform keys after super(...) returns, this map contains only the constructor sections even though the effective key list contains those additional keys. Calls through hasKey()/getKey() then disagree with getKeys()—in particular, ListDataStore.setRaw(String, ...) silently rejects such a key. The previous lazy implementation built the map through the virtual getKeys() accessor, so the thread-safe publication fix should retain that behavior or make overriding the accessor impossible.
Useful? React with 👍 / 👎.
| } catch (RuntimeException e) { | ||
| // The launch itself failed, e.g. Graphviz is not installed | ||
| return false; |
There was a problem hiding this comment.
If the first isDotAvailable() call is interrupted while waiting for an installed dot process, SpecsSystem.runProcess() throws a RuntimeException, but this catch converts it to false and the lazy value permanently caches Graphviz as unavailable. Other post-launch runtime failures are likewise hidden. As in renderDot(), only exceptions satisfying SpecsSystem.isLaunchFailure(e) should be treated as an absent installation; unrelated failures should propagate.
Useful? React with 👍 / 👎.
1 participant
Generic key defaults were lost because
setDefaultreturns a copied key. Return that copy, keep temporary-directory cleanup and cache-directory creation consistent, and remove the stale GuiHelper composite include.Regression coverage exercises default suppliers, temporary-directory lifecycle and concurrent store definitions/key maps. Existing optimized store implementations are retained.
Validation: SpecsUtils full check passes 6,656 tests with two skips. The jOptions full run has 17 legacy XML failures; an isolated run at the original base reproduces the same failing tests and exception identities. The XStream incompatibility warning remains, and these XML failures are not claimed fixed.