Pull requests #36 and #38

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Pull requests #36 and #38

Stefan Richthofer
Hello everybody,

given that Jython 2.7.1 was delayed so long (actually we're about to reach the planned release date for 2.7.2) I'd like to merge the pull requests

https://github.com/jythontools/jython/pull/36

https://github.com/jythontools/jython/pull/38

before the release. I think there is no risk to introduce new issues with these changes, because they really do simple stuff or just add a function.

Regarding #36:
Given that there was no uname in Jython before there cannot be (sane*) code that could be affected even if my implementation was faulty.

* code might be conditioned on uname not existing, but I regard it a no-goal to preserve bug-compatibility to old versions


Regarding #38:
Changes only concern if-branches that don't apply to Jython anyway. Actually these branches could have been removed completely (stuff that is conditioned onto e.g. os.name == 'nt'). I did not remove them because Jython code might want to hack around with them, e.g. by actively setting os.name to some value != 'java' (code I doubt to exist anyway). However for JyNI I need these branches additionally secured by a check for != 'java' for reasons that go too far to explain here (I can give details on the use case on demand).
So semantically these checks don't change anything for Jython at all; they even reduce the number of failing checks a bit, as os.name != 'java' usually wraps an entire block of several checks for various platforms not applying to Jython-case.

If there are no concerns I would check these into Jython someday next week. Presumably these commits would allow me to drop the JyNI-specific ctypes python code and use the CPython-bundled ctypes entirely. #36 is also required for JyOpenGL.

Regards

Stefan

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pull requests #36 and #38

Jeff Allen-2
I don't have any rights to this account. I see we invite pull requests
there: out of curiosity, is there a bridge back to hg?

I like the way GitHub gives us a build server (but not the way it's
always red).

J

Jeff Allen

On 30/04/2016 12:34, Stefan Richthofer wrote:

> Hello everybody,
>
> given that Jython 2.7.1 was delayed so long (actually we're about to reach the planned release date for 2.7.2) I'd like to merge the pull requests
>
> https://github.com/jythontools/jython/pull/36
>
> https://github.com/jythontools/jython/pull/38
>
> before the release. I think there is no risk to introduce new issues with these changes, because they really do simple stuff or just add a function.
>
> Regarding #36:
> Given that there was no uname in Jython before there cannot be (sane*) code that could be affected even if my implementation was faulty.
>
> * code might be conditioned on uname not existing, but I regard it a no-goal to preserve bug-compatibility to old versions
>
>
> Regarding #38:
> Changes only concern if-branches that don't apply to Jython anyway. Actually these branches could have been removed completely (stuff that is conditioned onto e.g. os.name == 'nt'). I did not remove them because Jython code might want to hack around with them, e.g. by actively setting os.name to some value != 'java' (code I doubt to exist anyway). However for JyNI I need these branches additionally secured by a check for != 'java' for reasons that go too far to explain here (I can give details on the use case on demand).
> So semantically these checks don't change anything for Jython at all; they even reduce the number of failing checks a bit, as os.name != 'java' usually wraps an entire block of several checks for various platforms not applying to Jython-case.
>
> If there are no concerns I would check these into Jython someday next week. Presumably these commits would allow me to drop the JyNI-specific ctypes python code and use the CPython-bundled ctypes entirely. #36 is also required for JyOpenGL.
>
> Regards
>
> Stefan
>
> ------------------------------------------------------------------------------
> Find and fix application performance issues faster with Applications Manager
> Applications Manager provides deep performance insights into multiple tiers of
> your business applications. It resolves application problems quickly and
> reduces your MTTR. Get your free trial!
> https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
> _______________________________________________
> Jython-dev mailing list
> [hidden email]
> https://lists.sourceforge.net/lists/listinfo/jython-dev
>


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pull requests #36 and #38

Jim Baker-2
On Sat, Apr 30, 2016 at 6:25 AM, Jeff Allen <[hidden email]> wrote:
I don't have any rights to this account. I see we invite pull requests
there: out of curiosity, is there a bridge back to hg?

We are just using GitHub PRs as a convenient place to review patches. Any commits against hg.python.org/jython occur via a manual commit.

We will be using https://github.com/jython/ for 2.7.2 and get off of hg. To do so, we first want to modernize our build process to use Maven dependencies (via Gradle); and filter out the extlib jars from the repo (so no more included binaries).

I like the way GitHub gives us a build server (but not the way it's
always red).

Darjus Loktevic set this up this CI earlier this year, and it has been very convenient in finding bugs for us to fix ;) such as the deadlock, now publication bug that has been blocking 2.7.1 (http://bugs.jython.org/issue2487)  I especially like how CircleCI works as a JVM with the turbo button (https://en.wikipedia.org/wiki/Turbo_button) turned off - very handy for finding bugs due to timing races.

- Jim

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pull requests #36 and #38

Jim Baker-2
In reply to this post by Stefan Richthofer
On Sat, Apr 30, 2016 at 5:34 AM, Stefan Richthofer <[hidden email]> wrote:
Hello everybody,

given that Jython 2.7.1 was delayed so long (actually we're about to reach the planned release date for 2.7.2)

Yes, I'm really well aware of how much we have slipped. On the other hand, this past week I had a chance to see a demo of some soon to be production code using our latest work, running on Twisted with SSL connections and against Java and Clojure libraries, all with quite reasonable performance due to no GIL.

So at this time, I'm hoping that we can complete by PyCon; or at least the PyCon sprints for those attending. See bit.ly/jython-triage-2_7_1 for outstanding bugs. Bugs of urgent/immediate severity block the release candidate; but we should include with respect to the high bugs as well, all of which have outstanding fixes to be applied.

The real challenge is the immediate bug ("the publication bug" for class/type mappings in http://bugs.jython.org/issue2487); it's something Darjus Loktevic and I have put in some time into, but so far, not resolved. But the final exam in the course I'm teaching is this Monday; and I just got back from the OpenStack Summit where I was kicking off a new project. So I personally have some time to squash that bug coming up real soon now.
 
I'd like to merge the pull requests

https://github.com/jythontools/jython/pull/36

https://github.com/jythontools/jython/pull/38

before the release. I think there is no risk to introduce new issues with these changes, because they really do simple stuff or just add a function.

+1. For committers, we generally follow a commit-then-review (CTR) process. The other committers haven't had a chance to review these PRs in advance - certainly not me -  but in general - simple stuff/adds a function has worked safely for us with CTR. Basically any committer should know when to reach out to another one for review of the tricky bits - that's why we were so designated :)

See my separate email sent on this same thread on how we actually merge such PRs. (tl;dr it's manual.)
 

Regarding #36:
Given that there was no uname in Jython before there cannot be (sane*) code that could be affected even if my implementation was faulty.

* code might be conditioned on uname not existing, but I regard it a no-goal to preserve bug-compatibility to old versions


Regarding #38:
Changes only concern if-branches that don't apply to Jython anyway. Actually these branches could have been removed completely (stuff that is conditioned onto e.g. os.name == 'nt'). I did not remove them because Jython code might want to hack around with them, e.g. by actively setting os.name to some value != 'java' (code I doubt to exist anyway). However for JyNI I need these branches additionally secured by a check for != 'java' for reasons that go too far to explain here (I can give details on the use case on demand).
So semantically these checks don't change anything for Jython at all; they even reduce the number of failing checks a bit, as os.name != 'java' usually wraps an entire block of several checks for various platforms not applying to Jython-case.

If there are no concerns I would check these into Jython someday next week. Presumably these commits would allow me to drop the JyNI-specific ctypes python code and use the CPython-bundled ctypes entirely. #36 is also required for JyOpenGL.

Regards

Stefan

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev


------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pull requests #36 and #38

Stefan Richthofer
Jim, thanks for your kind reply.
 
>we generally follow a commit-then-review (CTR) process
Alright. I was aware of this workflow implicitly so far. For #36 and #38 I was just looking for an explicit 'okay' even for save stuff, because it's in the hot shortly-before-release-phase (and it had been postulated that new features should go into 2.7.2, which was stated assuming less delay for 2.7.1 I suppose).
 
 
Gesendet: Sonntag, 01. Mai 2016 um 03:40 Uhr
Von: "Jim Baker" <[hidden email]>
An: "Stefan Richthofer" <[hidden email]>
Cc: "Jython Developers" <[hidden email]>
Betreff: Re: [Jython-dev] Pull requests #36 and #38
On Sat, Apr 30, 2016 at 5:34 AM, Stefan Richthofer <Stefan.Richthofer@...> wrote:
Hello everybody,

given that Jython 2.7.1 was delayed so long (actually we're about to reach the planned release date for 2.7.2)
 
Yes, I'm really well aware of how much we have slipped. On the other hand, this past week I had a chance to see a demo of some soon to be production code using our latest work, running on Twisted with SSL connections and against Java and Clojure libraries, all with quite reasonable performance due to no GIL.
 
So at this time, I'm hoping that we can complete by PyCon; or at least the PyCon sprints for those attending. See bit.ly/jython-triage-2_7_1 for outstanding bugs. Bugs of urgent/immediate severity block the release candidate; but we should include with respect to the high bugs as well, all of which have outstanding fixes to be applied.
 
The real challenge is the immediate bug ("the publication bug" for class/type mappings in http://bugs.jython.org/issue2487); it's something Darjus Loktevic and I have put in some time into, but so far, not resolved. But the final exam in the course I'm teaching is this Monday; and I just got back from the OpenStack Summit where I was kicking off a new project. So I personally have some time to squash that bug coming up real soon now.
 
I'd like to merge the pull requests

https://github.com/jythontools/jython/pull/36

https://github.com/jythontools/jython/pull/38

before the release. I think there is no risk to introduce new issues with these changes, because they really do simple stuff or just add a function.
 
+1. For committers, we generally follow a commit-then-review (CTR) process. The other committers haven't had a chance to review these PRs in advance - certainly not me -  but in general - simple stuff/adds a function has worked safely for us with CTR. Basically any committer should know when to reach out to another one for review of the tricky bits - that's why we were so designated :)
 
See my separate email sent on this same thread on how we actually merge such PRs. (tl;dr it's manual.)
 

Regarding #36:
Given that there was no uname in Jython before there cannot be (sane*) code that could be affected even if my implementation was faulty.

* code might be conditioned on uname not existing, but I regard it a no-goal to preserve bug-compatibility to old versions


Regarding #38:
Changes only concern if-branches that don't apply to Jython anyway. Actually these branches could have been removed completely (stuff that is conditioned onto e.g. os.name == 'nt'). I did not remove them because Jython code might want to hack around with them, e.g. by actively setting os.name to some value != 'java' (code I doubt to exist anyway). However for JyNI I need these branches additionally secured by a check for != 'java' for reasons that go too far to explain here (I can give details on the use case on demand).
So semantically these checks don't change anything for Jython at all; they even reduce the number of failing checks a bit, as os.name != 'java' usually wraps an entire block of several checks for various platforms not applying to Jython-case.

If there are no concerns I would check these into Jython someday next week. Presumably these commits would allow me to drop the JyNI-specific ctypes python code and use the CPython-bundled ctypes entirely. #36 is also required for JyOpenGL.

Regards

Stefan

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
Jython-dev@...
https://lists.sourceforge.net/lists/listinfo/jython-dev
------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z_______________________________________________ Jython-dev mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/jython-dev

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev
Reply | Threaded
Open this post in threaded view
|

Re: Pull requests #36 and #38

Darjus Loktevic
Hey Stefan,

That's great diligence. The changes look safe to me.

Cheers,
Darjus

On Sun, May 1, 2016 at 12:51 PM Stefan Richthofer <[hidden email]> wrote:
Jim, thanks for your kind reply.
 
>we generally follow a commit-then-review (CTR) process
Alright. I was aware of this workflow implicitly so far. For #36 and #38 I was just looking for an explicit 'okay' even for save stuff, because it's in the hot shortly-before-release-phase (and it had been postulated that new features should go into 2.7.2, which was stated assuming less delay for 2.7.1 I suppose).
 
 
Gesendet: Sonntag, 01. Mai 2016 um 03:40 Uhr
Von: "Jim Baker" <[hidden email]>
An: "Stefan Richthofer" <[hidden email]>
Cc: "Jython Developers" <[hidden email]>
Betreff: Re: [Jython-dev] Pull requests #36 and #38
On Sat, Apr 30, 2016 at 5:34 AM, Stefan Richthofer <Stefan.Richthofer@...> wrote:
Hello everybody,

given that Jython 2.7.1 was delayed so long (actually we're about to reach the planned release date for 2.7.2)
 
Yes, I'm really well aware of how much we have slipped. On the other hand, this past week I had a chance to see a demo of some soon to be production code using our latest work, running on Twisted with SSL connections and against Java and Clojure libraries, all with quite reasonable performance due to no GIL.
 
So at this time, I'm hoping that we can complete by PyCon; or at least the PyCon sprints for those attending. See bit.ly/jython-triage-2_7_1 for outstanding bugs. Bugs of urgent/immediate severity block the release candidate; but we should include with respect to the high bugs as well, all of which have outstanding fixes to be applied.
 
The real challenge is the immediate bug ("the publication bug" for class/type mappings in http://bugs.jython.org/issue2487); it's something Darjus Loktevic and I have put in some time into, but so far, not resolved. But the final exam in the course I'm teaching is this Monday; and I just got back from the OpenStack Summit where I was kicking off a new project. So I personally have some time to squash that bug coming up real soon now.
 
I'd like to merge the pull requests

https://github.com/jythontools/jython/pull/36

https://github.com/jythontools/jython/pull/38

before the release. I think there is no risk to introduce new issues with these changes, because they really do simple stuff or just add a function.
 
+1. For committers, we generally follow a commit-then-review (CTR) process. The other committers haven't had a chance to review these PRs in advance - certainly not me -  but in general - simple stuff/adds a function has worked safely for us with CTR. Basically any committer should know when to reach out to another one for review of the tricky bits - that's why we were so designated :)
 
See my separate email sent on this same thread on how we actually merge such PRs. (tl;dr it's manual.)
 

Regarding #36:
Given that there was no uname in Jython before there cannot be (sane*) code that could be affected even if my implementation was faulty.

* code might be conditioned on uname not existing, but I regard it a no-goal to preserve bug-compatibility to old versions


Regarding #38:
Changes only concern if-branches that don't apply to Jython anyway. Actually these branches could have been removed completely (stuff that is conditioned onto e.g. os.name == 'nt'). I did not remove them because Jython code might want to hack around with them, e.g. by actively setting os.name to some value != 'java' (code I doubt to exist anyway). However for JyNI I need these branches additionally secured by a check for != 'java' for reasons that go too far to explain here (I can give details on the use case on demand).
So semantically these checks don't change anything for Jython at all; they even reduce the number of failing checks a bit, as os.name != 'java' usually wraps an entire block of several checks for various platforms not applying to Jython-case.

If there are no concerns I would check these into Jython someday next week. Presumably these commits would allow me to drop the JyNI-specific ctypes python code and use the CPython-bundled ctypes entirely. #36 is also required for JyOpenGL.

Regards

Stefan

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
Jython-dev@...
https://lists.sourceforge.net/lists/listinfo/jython-dev
------------------------------------------------------------------------------ Find and fix application performance issues faster with Applications Manager Applications Manager provides deep performance insights into multiple tiers of your business applications. It resolves application problems quickly and reduces your MTTR. Get your free trial! https://ad.doubleclick.net/ddm/clk/302982198;130105516;z_______________________________________________ Jython-dev mailing list [hidden email] https://lists.sourceforge.net/lists/listinfo/jython-dev
------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev

------------------------------------------------------------------------------
Find and fix application performance issues faster with Applications Manager
Applications Manager provides deep performance insights into multiple tiers of
your business applications. It resolves application problems quickly and
reduces your MTTR. Get your free trial!
https://ad.doubleclick.net/ddm/clk/302982198;130105516;z
_______________________________________________
Jython-dev mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/jython-dev