Skip to content

Conversation

@sklemer1
Copy link
Contributor

@sklemer1 sklemer1 commented Aug 28, 2018

  • silence zenity
  • usable without dbus
  • correct shebang
  • detect python3 (with some from __future__ we could make it work with
    python2 but is this necessary?)

This is not tested very thorowly.

- silence zenity
- usable without dbus
- correct shebang
- detect python3 (with some from __future__ we could make it work with
python2 but is this necessary?)
@restena-sw
Copy link
Contributor

You've sent the pull request against master. Do you want this to be part of the 2.0.x series? Then it should go into branch release_2_0 instead.

TBH, when I read "not tested very thoroughly" maybe it's better to merge it by a future 2.0.1 time and not during our short beta phase before going to prod...?

@sklemer1
Copy link
Contributor Author

That's why it's not against release_2_0 but master. I can also make it to apply against 2.0 -- the only difference should be the #114 shebang-line. I don't know how and where to test this without a large audience and many different distros. On my 2 test distros (Debian and Ubuntu) and some mockup scenarios it was working fine. I don't know how much this was tested before as the missing shebang was a real bummer....

@restena-sw
Copy link
Contributor

These things are rather thoroguhly tested by @twoln - he has many distros on his list. But then he is so knowledgable that he probably always typed in "python ..." and didn't realise the shebang issue.
Anyway, the content of this pull request is for him in entirety; it's all his code.

@sklemer1 sklemer1 mentioned this pull request Sep 14, 2018
if q.returncode == 0:
self.graphics = 'zenity'
if not debug_on:
self.stderr_redir=subprocess.DEVNULL
Copy link
Contributor

Choose a reason for hiding this comment

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

DEVNULL exists since 3.3. So no 2.7 support. We should define which python versions we will support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops. Good catch, thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way I read these modification it looks like dropping python 2.7 altogether. As I said in the other thread, I would be reluctant do do that at this stage. Do you really think that the issues we may now have are so bad? I did not observe them in my tests.

Fixing the problem with zenity output is great, of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's easy to change it for py2-support if we decide to want it -- we could just open /dev/null directly or pipe it somewhere else.


def run_installer():
global debug_on
if sys.version_info.major < 3:
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe, we should also test older python3 versions with sys.version_info.minor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you use features from later versions, definitely...

@twoln
Copy link
Contributor

twoln commented Oct 25, 2018

I think all of these have been implemented. OK to close?

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.

https://github.com/GEANT/CAT/commit/77954ee3f107b6f392291468be843e04adc23d2d

5 participants