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

C-level output redirection on windows #126

Merged
merged 2 commits into from
Mar 1, 2022
Merged

Conversation

anubhav-cs
Copy link
Contributor

Currently, we cannot capture c-level output handler on windows. This causes an error when we when attempt to import command.py. Hence, I cannot ground PDDL on windows.

There are two resolutions for this. The second resolution is what this pull request does.

  1. stdout_redirect is the context manager which requires the c-level output handler c_stdout, but we don't use it anywhere. Hence, we can resolve this by simply removing stdout_redirect and the portion of code which retrieves the handler. That is, we remove everything from

    libc = ctypes.CDLL(None)
    until end of file.

  2. In case, we want to keep stdout_redirect, I have added the code to retrieve the handler on Windows. The program would fall back to original routine if sys.platform is anything other than win32.

Currently, we cannot capture c-level output handler on windows. This causes error when we attempt to ground a PDDL on Windows. There are two resolutions for this.

1. `stdout_redirect` is the function which requires the c-level output handler, but we don't call it anywhere.  Hence, we can resolve this by simply removing the function and the portion of code which retrieves the handler. That is, we remove everything from https://github.com/aig-upf/tarski/blob/80a33dc5fe6ad2602b40b1c69f758b91188dd8e1/src/tarski/utils/command.py#L72 until end of file.

2. In case, we need to keep the function `stdout_redirect`, I have added the code to retrieve the handler on Windows. The program would fall back to original routine if `sys.platform` is anything other than `win32`.
@miquelramirez
Copy link
Member

Looks good, thanks @anubhav-cs !

@gfrances
Copy link
Member

Thanks @anubhav-cs. I'm having a look at the code. Indeed, as you say, we're not using stdout_redirector anymore. This was used in the past in some integration with an SDD solver that we were exploring, but that we finally dropped. It was probably an oversight on my part that I didn't remove this code.

BTW I'm seeing that the ctypes.CDLL code also caused some problems in mac os in the past.
Since we're not using this aanymore, I think I would rather go with your option (1) and remote the entire portion of the code, hence simplifying everything. @miquelramirez , would you be happy with that?

@miquelramirez
Copy link
Member

I can't see any obvious side effects, so I would say that 1) seems to be the way to go.

Could you @anubhav-cs ammend the PR with the desired solution? Thanks!

@miquelramirez miquelramirez self-assigned this Feb 28, 2022
@anubhav-cs
Copy link
Contributor Author

Hi @miquelramirez

I have updated the PR.

@gfrances gfrances merged commit 134f59e into aig-upf:devel Mar 1, 2022
@gfrances
Copy link
Member

gfrances commented Mar 1, 2022

Thanks Anu!

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