aig-upf / tarski

Tarski - An AI Planning Modeling Framework
Apache License 2.0
59 stars 19 forks source link

Adds logic to use the correct stdout id on MacOS when environment is "darwin" #97

Closed mejrpete closed 3 years ago

mejrpete commented 3 years ago

I'm not sure how interested you all are in supporting tarski on macos. I know that a few people working with it now do development work with it locally on macs, although I believe it's generally deployed in linux environments.

Previously, tests (as well as actual use) would fail on macos due to a mismatch between the expected cytpe ID for stdout (stdout -- as it typically is on linux) and the actual ctype ID (__stdoutp). This fix just directly assigns the string to be __stdoutp in the case when we are in a darwin environment.

As far as I can tell, this is the best way to do this that's straightforward, since it appears that stdout is generally a macro, rather than a variable. It appears macos is the weird one out here, so it seems to make sense to just fix the problem directly.

codecov-io commented 3 years ago

Codecov Report

Merging #97 into master will decrease coverage by 0.50%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   98.30%   97.80%   -0.51%     
==========================================
  Files          47       48       +1     
  Lines        3014     3054      +40     
  Branches      114      121       +7     
==========================================
+ Hits         2963     2987      +24     
- Misses         41       51      +10     
- Partials       10       16       +6     
Impacted Files Coverage Δ
tests/io/common.py 60.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2d57d46...8f9e85c. Read the comment docs.

gfrances commented 3 years ago

Hi @mejrpete , thanks for the PR. We are definitely interested in supporting macos, though I have to say that at the moment I don't have access to any such machine :-)

But this looks definitely good. For the record, it might be good to add osx testing to our travis tests, e.g. as described in https://docs.travis-ci.com/user/multi-os/.