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

refactoring : removed explicits os.fork(), exceptions are propagated from child process to parent #1228

Closed

Conversation

alalazo
Copy link
Member

@alalazo alalazo commented Jul 12, 2016

Modifications
  • removed explicit os.fork() in favor of multiprocessing.Process
  • log_output logic has been modified and does not require calls to sys.settrace anymore
  • we can throw from Package.do_install.build_process and the exceptions are propagated to parent process
Changes in log_ouput

In develop :

  • on __enter__ the parent process forks , set his frame explicitly to avoid executing twice the same code (quite fragile to re-positioning of the with statement) , reads from a pipe and writes to file
  • on __enter__ the child does the necessary to write to a pipe and then returns for execution

In the PR:

  • a "daemonic" child is spawned in the acquire method and is set to read from the pipe
  • the context manager just changes where the parent writes when needed
  • everything is cleaned in the release method
Notes

In #1186 I wanted to be able to forward exceptions to parent to have a neater workflow in do_install. As I saw a couple of TODOs referring to the same issue, I extracted this PR. This has also the great advantage of not tweaking with sys.settrace and frames (which is a major pain for the debugger sometimes).

@alalazo alalazo changed the title refactoring : removed explicits os.fork(), exceptions are propagated from child process to parent [WIP] refactoring : removed explicits os.fork(), exceptions are propagated from child process to parent Jul 12, 2016
@alalazo alalazo changed the title [WIP] refactoring : removed explicits os.fork(), exceptions are propagated from child process to parent refactoring : removed explicits os.fork(), exceptions are propagated from child process to parent Jul 13, 2016
@alalazo
Copy link
Member Author

alalazo commented Jul 13, 2016

Ready to be reviewed and tested

@tgamblin
Copy link
Member

@alalazo: this is awesome! I haven't had a chance to look at it in detail but will. This also removes one of the obstacles to getting Spack working on Windows.

@goxberry Does this work for you on Windows?

@alalazo
Copy link
Member Author

alalazo commented Jul 13, 2016

@tgamblin I would really like this to be merged, but to be fair I have to tell you that in #1186 spurious fails in test/install.py appeared possibly due to this change-set. I was not able to reproduce them outside Travis and I have the impression that the tests themselves may be flawed somehow, but it is worth a double check before merging.

@alalazo
Copy link
Member Author

alalazo commented Jul 13, 2016

@tgamblin I got the point failing spuriously here : I'll write a fix a ping you when done.

@alalazo
Copy link
Member Author

alalazo commented Jul 13, 2016

@tgamblin Done : now it should be 99,9% safe 😄

@alalazo
Copy link
Member Author

alalazo commented Jul 14, 2016

@tgamblin I pushed some cosmetic changes to this in #1186 , let me know if you want them ported here

@goxberry
Copy link
Contributor

@tgamblin Will check with my co-worker who uses Windows (he was the original reason for the request) and get back to you

@tgamblin
Copy link
Member

@goxberry: I looked into this. There are a couple more things that probably need to happen:

  1. Set use_tmp_stage to False in __init__.py.
  2. In lib/spack/env, there is really only one file with code in it: cc. The other files in lib/spack/env and its subdirectories are symlinks to that. You probably need to overwrite every file that is not cc with the contents of cc. Apparently by default if you clone on windows, a symlink comes down as a file with the path to its target in it, which is not really what we want. Not sure how best to get around that right now, but this could be a potential workaround.

…ing/removed_explicit_fork

Conflicts:
	lib/spack/spack/package.py
@alalazo
Copy link
Member Author

alalazo commented Aug 11, 2016

Closing as it will be difficult to maintain this alongside #1186

@alalazo alalazo closed this Aug 11, 2016
@tgamblin
Copy link
Member

If it were merged would it be easy or is it a fundamental incompatibility?

@alalazo
Copy link
Member Author

alalazo commented Aug 11, 2016

@tgamblin No fundamental incompatibility : all the features here are also in #1186 except that maintaining both will create conflicts (due to synchronization with develop) that needs to be managed.

In the end I think it will be easier to re-create something like #1228 if you need just that rather than continue with this PR.

@tgamblin
Copy link
Member

@alalazo: ok -- sounds good. Sorry for being slow!

@alalazo
Copy link
Member Author

alalazo commented Aug 11, 2016

@tgamblin No worries. I closed it as leaving as it was was just adding noise to the list of PRs

olupton pushed a commit to olupton/spack that referenced this pull request Feb 7, 2022
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.

None yet

3 participants