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

Don't pass invalid ICE_CONFIG to Ice (fix #10051). #3131

Merged
merged 1 commit into from
Oct 28, 2014

Conversation

bpindelski
Copy link

This PR fixes https://trac.openmicroscopy.org/ome/ticket/10051.

To test, follow the steps from the ticket, with a tiny modification:

  • Start the server
  • Login in via bin/omero login
  • Set the ICE_CONFIG env var (export ICE_CONFIG=bin/omero sessions file``)
  • Log out (bin/omero logout)
  • Stop the server
  • Try to start the server
  • Verify that no error message appears

This should also work when ICE_CONFIG is set to any invalid file/directory.

@dominikl
Copy link
Member

No error message, server starts fine, good to merge.

@joshmoore
Copy link
Member

Makes sense to me. @bpindelski, did you happen to take a look at the interaction with the lines:

3b5c3848 components/tools/OmeroPy/src/omero/cli.py (jmoore           2008-05-27 14:11:41 +0000 1245)     # Modiying the run-time environment
3b5c3848 components/tools/OmeroPy/src/omero/cli.py (jmoore           2008-05-27 14:11:41 +0000 1246)     old_ice_config = os.getenv("ICE_CONFIG")
3b5c3848 components/tools/OmeroPy/src/omero/cli.py (jmoore           2008-05-27 14:11:41 +0000 1247)     os.unsetenv("ICE_CONFIG")

Guess to some extent it's suprising that ICE_CONFIG is set if unsetenv is called. But it might be that your solution of only unsetting if the file doesn't exist is more generally applicable.

@sbesson, that may just need to be something we look into later, but any comments before merging welcome.

@bpindelski
Copy link
Author

@joshmoore The os.unsetenv was my initial implementation and it didn't work. Only the current approach seems to be doing the trick.

@joshmoore
Copy link
Member

Interesting. Then there must be something I didn't understand about unsetenv. Merging, and @sbesson and I can keep in mind that this is perhaps a better long-term solution. Thanks!

joshmoore added a commit that referenced this pull request Oct 28, 2014
Don't pass invalid ICE_CONFIG to Ice (fix #10051).
@joshmoore joshmoore merged commit a595b2b into ome:develop Oct 28, 2014
@bpindelski bpindelski deleted the 10051_ice_config branch October 28, 2014 13:39
@bpindelski
Copy link
Author

--no-rebase

@sbesson sbesson added this to the 5.1.0-m2 milestone Nov 26, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants