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

Modifications to permit running on MacOS #207

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Conversation

jmeyers314
Copy link
Collaborator

No description provided.

@jmeyers314 jmeyers314 force-pushed the tickets/DM-41369 branch 3 times, most recently from af3c966 to 123e869 Compare October 26, 2023 22:30
@jfcrenshaw
Copy link
Collaborator

LGTM.

One question: Are the stdout and stderr arguments you added to runProgram required for Mac compatibility, or are you just adding those because they're useful to have in certain contexts?

@jfcrenshaw jfcrenshaw self-requested a review October 27, 2023 03:52
jfcrenshaw
jfcrenshaw previously approved these changes Oct 27, 2023
@jmeyers314
Copy link
Collaborator Author

Are the stdout and stderr arguments you added to runProgram required for Mac compatibility

Actually, it just occurred to me that this is probably a generic shlex compatibility problem, and not just on MacOS. For example, if you try subprocess.call(shlex.split("ls &> ls.log"), shell=False), instead of piping stderr/stdout to ls.log, ls complains that it can't find the files &> and ls.log. I guess redirection doesn't work when handing subprocess.call a list instead of a string.

There's at least one line in ts_imsim where this matters: https://github.com/lsst-ts/ts_imsim/blob/develop/python/lsst/ts/imsim/closedLoopTask.py#L1191. I'll add that to this ticket and look around for any others.

So thanks for asking!

@jfcrenshaw jfcrenshaw self-requested a review October 27, 2023 15:53
@jfcrenshaw jfcrenshaw dismissed their stale review October 27, 2023 15:54

See note about shlex stdout/stderr piping above

Copy link
Collaborator

@jfcrenshaw jfcrenshaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. You just need to rebase and bump the version patch number once more.

@jmeyers314 jmeyers314 merged commit dfa6330 into develop Oct 31, 2023
3 checks passed
@jmeyers314 jmeyers314 deleted the tickets/DM-41369 branch October 31, 2023 21:04
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