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

Attempt to use null separation when invoking 'env' #684

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cottsay
Copy link
Member

@cottsay cottsay commented Jan 17, 2025

This approach eliminates the heuristics for handling newlines in environment variables on platforms where 'env' supports null separation via the '-0' flag. The 'sh' shell will first attempt to use null separation and will fall back to the prior behavior.

The existing heuristics make some pretty critical assumptions about environment variable naming that can misbehave in extremely cryptic ways.

A specific example is that the regular expression used to determine if a line contains a valid environment variable name is based on the characters that Bourne shells accept when setting environment variables, but the set of acceptable characters in Linux is wider and the shells may still pass those variables through to subprocesses even though they don't support setting new variables with such names. The existing code will treat these variables as a continuation of the previous variable. If the previous variable is one that we depend on, such as AMENT_PREFIX_PATH, we can see cryptic problems during the build that don't appear to be related to colcon.

The heuristics were added in #128 to handle multi-line environment variables.

@cottsay cottsay added the bug Something isn't working label Jan 17, 2025
@cottsay cottsay self-assigned this Jan 17, 2025
@cottsay cottsay force-pushed the cottsay/null-sep-env branch from 01e19fe to 33b30ae Compare January 17, 2025 22:36
This approach eliminates the heuristics for handling newlines in
environment variables on platforms where 'env' supports null separation
via the '-0' flag. The 'sh' shell will first attempt to use null
separation and will fall back to the prior behavior.
@cottsay cottsay force-pushed the cottsay/null-sep-env branch from 33b30ae to 5fc80a7 Compare January 17, 2025 22:42
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 73.07692% with 7 lines in your changes missing coverage. Please review.

Project coverage is 87.18%. Comparing base (a74fa97) to head (5fc80a7).

Files with missing lines Patch % Lines
colcon_core/shell/__init__.py 78.94% 4 Missing ⚠️
colcon_core/shell/sh.py 57.14% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
- Coverage   87.28%   87.18%   -0.10%     
==========================================
  Files          68       68              
  Lines        3949     3973      +24     
  Branches      760      763       +3     
==========================================
+ Hits         3447     3464      +17     
- Misses        396      403       +7     
  Partials      106      106              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

christophebedard pushed a commit to christophebedard/action-ros-ci-almalinux-test that referenced this pull request Jan 22, 2025
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
christophebedard pushed a commit to christophebedard/action-ros-ci-almalinux-test that referenced this pull request Jan 22, 2025
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
christophebedard pushed a commit to christophebedard/action-ros-ci-almalinux-test that referenced this pull request Jan 22, 2025
Signed-off-by: Christophe Bedard <christophe.bedard@apex.ai>
@christophebedard
Copy link
Contributor

Just to mention it here:

I tested this and it fixes ros2/rosidl_typesupport#162, see ros2/rosidl_typesupport#162 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

2 participants