Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI helper improvements #776

Merged
merged 17 commits into from
Jan 23, 2023
Merged

CLI helper improvements #776

merged 17 commits into from
Jan 23, 2023

Conversation

rcoup
Copy link
Member

@rcoup rcoup commented Jan 22, 2023

Description

Fixes and improvements for the CLI helper:

  • extend logging and debugging for both the C & Python helper code
  • fix issue with Apple Silicon and kart-initiated subprocesses
  • fix issue passing large environments/contexts to the background helper
  • improve searching logic in the C helper for the sibling kart_cli binary
  • enable the CLI helper by default in CI builds for macOS & Linux.
  • code-style improvements to the C helper code; enforce C11 and compile with warnings as errors
  • add helper logging in E2E tests

To skip using the background helper, setting KART_USE_HELPER=0 in the environment continues to work.

Related links:

Checklist:

  • Have you reviewed your own change?
  • Have you included test(s)?
  • Have you updated the changelog?

craigds and others added 17 commits January 16, 2023 16:09
* set C standard to C11
* create kart/kart_cli symlinks in dev, not just bundle
* enable debug via `-DCMAKE_BUILD_TYPE=Debug`
Enable via -DCMAKE_BUILD_TYPE=Debug at build-time and setting
KART_HELPER_DEBUG=1 in the environment.
Search for the kart executable via OS-specific methods if possible,
falling back to argv[0]. Then look for sibling kart_cli. Then try again
after re-resolving symlinks.
If Kart is built with -DCLI_HELPER=ON then make it on by default.
* enable test-run helper logging and dump log file after e2e tests
* turn off/on explicitly to deal with helper mode being default enabled
* handle semget() flags correctly per-OS
* add pid to log messages
* make semnum a constant
* build with -Wall & -Werror
* write to helper log from helper & children, resolve to absolute path so that cwd changes don't break
* more detailed logging around forks/pids, and exit/semaphore messaging
* `union semun` is defined differently on Linux & macOS... even though they're effectively the same, reflect accurately

On macOS/arm64, it's critical to define the final argument to semctl()
as variadic via ctypes (ie: don't define it), because the calling
convention differs between fixed and variadic arguments.
python/cpython#92892
we set it to ignore in the helper itself, but if the execution
kart process invokes a subprocess (eg: git) and interacts via
asyncio, then the subprocess exit code is lost on Linux.

Reset it to the default after the fork happens.
Means local development builds don't have gotchas wrt code-reloading,
but CI & release builds include it. Add a note to the contributing docs.
@olsen232 olsen232 merged commit 241c67d into master Jan 23, 2023
@olsen232 olsen232 deleted the fix-cli-helper branch January 23, 2023 02:19
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.

3 participants