Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Friday, 28 February 2020 10:42:54 PST Thiago Macieira wrote: > On Thursday, 27 February 2020 11:50:02 PST Simon Hausmann wrote: > > Hi, > > > > Declarative in 5.15 (5.14?) has public API taking a std::function and that > > appears to be okay so far :) > > Good to know. I see it in two public headers in 5.14. > > We've just found out that std::mutex is missing on a non-insignificant > platform: MinGW (at least some builds of it). I just hope that people using > those platforms simply hadn't skipped qtdeclarative in 5.14. Pushed as https://codereview.qt-project.org/c/qt/qtbase/+/292539 The previous four changes in the same line of fixes need to be approved before this gets in, though. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel System Software Products ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Thursday, 27 February 2020 11:50:02 PST Simon Hausmann wrote: > Hi, > > Declarative in 5.15 (5.14?) has public API taking a std::function and that > appears to be okay so far :) Good to know. I see it in two public headers in 5.14. We've just found out that std::mutex is missing on a non-insignificant platform: MinGW (at least some builds of it). I just hope that people using those platforms simply hadn't skipped qtdeclarative in 5.14. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel System Software Products ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
Hi, Declarative in 5.15 (5.14?) has public API taking a std::function and that appears to be okay so far :) Simon > Am 27.02.2020 um 18:53 schrieb Thiago Macieira : > > On Monday, 24 February 2020 03:07:00 PST Lars Knoll wrote: >>> On 21 Feb 2020, at 18:49, Thiago Macieira >>> wrote: > On Friday, 21 February 2020 08:39:57 PST Volker Hilsheimer wrote: >>> I’m fine with that, and as communicated by Jani, deprecations can be done even with feature freeze in effect, with approval from module maintainer (up until Beta1; after that only with Lars’ approval). >>> >>> >>> Do note the flip side of the coin: adding a new API to replace the >>> overriding. >> >> Right, it’s a bit more than just deprecating a method. But looking at your >> proposal I’m ok with the change for 5.15, if you can get it done quickly. > > If it's just a std::function we should be ok. The problem is > going > to be determining whether std::function is acceptable in 5.15 for all our > compilers. > > -- > Thiago Macieira - thiago.macieira (AT) intel.com > Software Architect - Intel System Software Products > > > > ___ > Development mailing list > Development@qt-project.org > https://lists.qt-project.org/listinfo/development ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Monday, 24 February 2020 03:07:00 PST Lars Knoll wrote: > > On 21 Feb 2020, at 18:49, Thiago Macieira > > wrote: > > On Friday, 21 February 2020 08:39:57 PST Volker Hilsheimer wrote: > > > >> I’m fine with that, and as communicated by Jani, deprecations can be > >> done > >> even with feature freeze in effect, with approval from module maintainer > >> (up until Beta1; after that only with Lars’ approval). > > > > > > Do note the flip side of the coin: adding a new API to replace the > > overriding. > > Right, it’s a bit more than just deprecating a method. But looking at your > proposal I’m ok with the change for 5.15, if you can get it done quickly. If it's just a std::function we should be ok. The problem is going to be determining whether std::function is acceptable in 5.15 for all our compilers. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel System Software Products ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
> On 21 Feb 2020, at 18:49, Thiago Macieira wrote: > > On Friday, 21 February 2020 08:39:57 PST Volker Hilsheimer wrote: >> I’m fine with that, and as communicated by Jani, deprecations can be done >> even with feature freeze in effect, with approval from module maintainer >> (up until Beta1; after that only with Lars’ approval). > > Do note the flip side of the coin: adding a new API to replace the overriding. Right, it’s a bit more than just deprecating a method. But looking at your proposal I’m ok with the change for 5.15, if you can get it done quickly. Cheers, Lars ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Friday, 21 February 2020 08:53:33 PST Oswald Buddenhagen wrote:
> On Fri, Feb 21, 2020 at 08:29:54AM -0800, Thiago Macieira wrote:
> >can we deprecate setupChildProcess() in Qt 5.15 and *remove it* in 6.0?
>
> at least, we should. ;)
> but let's bikeshed the name of the callback setter, which needs to be
> added at the same time -
> setChildProcess{Modifier,Initializer,Setup,...}() ^^
I even thought of making it a signal, like QCoreApplication::aboutToQuit:
QProcess::childAboutToStart(). That allows for N hooks to be considered, not
just one, and derived classes like KProcess could insert their own hooks
independent of their users.
But that is probably a bad idea, as the machinery for emitting signals isn't
designed to be async-signal-safe. For any receiver in another thread, there's
also a mutex, which we can't assume we can lock.
If a derived class needs its own child code, it could override the setter and
wrap calling the user's inside its own.
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel System Software Products
___
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Friday, 21 February 2020 08:39:57 PST Volker Hilsheimer wrote: > I’m fine with that, and as communicated by Jani, deprecations can be done > even with feature freeze in effect, with approval from module maintainer > (up until Beta1; after that only with Lars’ approval). Do note the flip side of the coin: adding a new API to replace the overriding. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel System Software Products ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Fri, Feb 21, 2020 at 08:29:54AM -0800, Thiago Macieira wrote:
can we deprecate setupChildProcess() in Qt 5.15 and *remove it* in 6.0?
at least, we should. ;)
but let's bikeshed the name of the callback setter, which needs to be
added at the same time -
setChildProcess{Modifier,Initializer,Setup,...}() ^^
___
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
> On 21 Feb 2020, at 17:29, Thiago Macieira wrote: > > On Tuesday, 18 February 2020 15:44:24 PST Thiago Macieira wrote: >> $ grep -r 'public QProcess' >> kcoreaddons/src/lib/io/kprocess.h:class KCOREADDONS_EXPORT KProcess : public >> QProcess >> khtml/src/java/kjavaprocess.h:class KJavaProcess : public QProcess //QObject >> kpty/src/kpty.cpp:class UtemptProcess : public QProcess >> kwin/utils.h:class KWIN_EXPORT Process : public QProcess >> >> The first two do not override setupChildProcess, the latter two do. > > Do note that the current check means we can't tell that KProcess did not > override setupChildProcess. That means any and ALL users of KProcess will > need > to revert to the older, slower, thread-unsafe, non-pidfd implementation. > > So let me ask again: can we deprecate setupChildProcess() in Qt 5.15 and > *remove it* in 6.0? I’m fine with that, and as communicated by Jani, deprecations can be done even with feature freeze in effect, with approval from module maintainer (up until Beta1; after that only with Lars’ approval). And I do assume you will approve :) Volker ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Tuesday, 18 February 2020 15:44:24 PST Thiago Macieira wrote: > $ grep -r 'public QProcess' > kcoreaddons/src/lib/io/kprocess.h:class KCOREADDONS_EXPORT KProcess : public > QProcess > khtml/src/java/kjavaprocess.h:class KJavaProcess : public QProcess //QObject > kpty/src/kpty.cpp:class UtemptProcess : public QProcess > kwin/utils.h:class KWIN_EXPORT Process : public QProcess > > The first two do not override setupChildProcess, the latter two do. Do note that the current check means we can't tell that KProcess did not override setupChildProcess. That means any and ALL users of KProcess will need to revert to the older, slower, thread-unsafe, non-pidfd implementation. So let me ask again: can we deprecate setupChildProcess() in Qt 5.15 and *remove it* in 6.0? -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel System Software Products ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Tuesday, 18 February 2020 03:17:38 PST Edward Welbourne wrote:
> Thiago Macieira (17 February 2020 20:35) wrote:
> > Sorry, this just occurred to me. This is a request for 5.15 feature freeze
> > exception.
> >
> > Re: https://bugreports.qt.io/browse/QTBUG-17331
>
> Closed: 30-07-2013 03:38, >9 years ago.
Because I had no clean way of implementing it until a couple of months ago,
when Linux 5.4 was released.
> > Re: 97645478de3ceffce11f58eab140c4c775e48be5
> > ("QProcess: use FFD_USE_FORK when the class is not QProcess itself")
>
> Committed to 5.15, has not appeared in any release yet.
> Modifies (only) corelib/io/qprocess_unix.cpp, only internal to
> QProcessPrivate::startProcess(); no API change.
This commit shows the consequence of the existing API.
> > I don't think we can remove the ability to run user code in the child
> > process. But can we remove the virtual? Can we just have a callback
> > like on Windows?
>
> I take it that would involve an API change.
> I see QProcess has differences in API between MS and Unix.
> Reducing that inconsistency would be good.
Correct, an API change. We can add more functionality to QProcess to minimise
the need to have user functions run, but that's not what I am talking about
here. We can let the native API continue to exist. All I want is that the Unix
one be by way of a setter of a callback, instead of by derivation.
> > Cons:
> > - breaks feature freeze, adds SIC for Qt 6
>
> Please clarify: if we include some new API in 5.15 to support this, what
> will be the SiC change in Qt 6 ?
The idea is to deprecate setupChildProcess in 5.15 so we can remove it 6.0.
The removal is the SIC part.
> > - of the 6 times I can find of QProcess being derived from in Qt & Qt
> >
> > Creator, 5 are to override setupChildProcess anyway and the last one
> > is in tst_QProcess
>
> Does anyone have sources from outside Qt project that are currently
> forced to derive from QProcess for the sake of this ? If so, please try
> to work out how Thiago's change would affect you and give the rest of us
> a summary.
Here are a couple more, searching KDE sources:
$ grep -r 'public QProcess'
kcoreaddons/src/lib/io/kprocess.h:class KCOREADDONS_EXPORT KProcess : public
QProcess
khtml/src/java/kjavaprocess.h:class KJavaProcess : public QProcess //QObject
kpty/src/kpty.cpp:class UtemptProcess : public QProcess
kwin/utils.h:class KWIN_EXPORT Process : public QProcess
The first two do not override setupChildProcess, the latter two do.
UtemptProcess needs to set up stdin, stdout and stderr to a specific file
descriptor, which comes from openpty(3), whereas the Utils::Process is just
unblocking two signals that I imagine KWin blocks for its own purposes.
That adds to:
qtbase/src/corelib/doc/snippets/code/src_corelib_io_qprocess.cpp
qtbase/tests/auto/corelib/io/qprocess/tst_qprocess.cpp
qtbase/tests/auto/corelib/tools/qsharedpointer/externaltests.cpp
qt-creator/src/libs/ssh/sshprocess.h
qt-creator/src/libs/utils/qtcprocess.h
qt-creator/src/libs/utils/synchronousprocess.cpp
The documentation example is trying to document why one would want to override
the function and it's showing an example of chroot()[1]. The QSharedPointer
code attempts to reset stdin to /dev/tty. SshProcess and
TerminalControllingProcess are just doing setsid(). QtcProcess is doing a nice
of 5[2].
The case of unblocking signals, closing all extraneous file descriptors,
setsid()/setpgrp(), detaching from the controlling TTY, etc., we can add flags
for and implement directly in QProcess without the user having to override
anything. The other cases seem too specific to add flags for and require a
callback.
[1] setting umask to 0 seems wrong. I'd set it to 022 or 077.
[2] the qWarning after it failed is a deadlock magnet => fixed in
https://codereview.qt-project.org/c/qt-creator/qt-creator/+/29
--
Thiago Macieira - thiago.macieira (AT) intel.com
Software Architect - Intel System Software Products
___
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Tuesday, 18 February 2020 04:11:01 PST Christian Kandeler wrote: > In qbs, we use it to call setpgid() with the id of the newly created > process. But I don't understand why that should matter at all: Whether we > inject the code via an overriden virtual or a std::function is purely a > question of implementation technique, is it not? How can there possibly be > a functional difference? The difference is how you communicate to QProcess that you want to run extra code. Right now, you do this by deriving from QProcess and overriding setupChildProcess. That means QProcessPrivate::start has no proper way of knowing when you did so and must assume that you do want that, preventing us from having some optimisations. See the two commits I've just uploaded to QProcess 5.15: they are not allowed if user code is running. I'm asking for a way that we can be certain that you want to run code. It will clean up your code and QProcess's. And if we like it, we can have a flag saying "my function is async-signal-safe too", which will allow the optimisations and the use code. -- Thiago Macieira - thiago.macieira (AT) intel.com Software Architect - Intel System Software Products ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On Tue, 18 Feb 2020 11:17:38 + Edward Welbourne wrote: > > - of the 6 times I can find of QProcess being derived from in Qt & Qt > > Creator, 5 are to override setupChildProcess anyway and the last one > > is in tst_QProcess > > Does anyone have sources from outside Qt project that are currently > forced to derive from QProcess for the sake of this ? If so, please try > to work out how Thiago's change would affect you and give the rest of us > a summary. In qbs, we use it to call setpgid() with the id of the newly created process. But I don't understand why that should matter at all: Whether we inject the code via an overriden virtual or a std::function is purely a question of implementation technique, is it not? How can there possibly be a functional difference? Christian ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
On 2/18/20 12:17 PM, Edward Welbourne wrote: > We would presumably want to subsequently rationalise the MS and Unix > APIs to have a common form, instead of having naked MS types in the API > for one and something else for the other. That would mean deprecating > the existing MS-specific API in favour of a cross-platform API at some > point. The API in question is to give the user the possibility to tweak the native API call. I don't see how this can be ever "crossplatformified" unless you mean passing void*. >> Alternative: add the API to Qt 6 and deprecate setupChildProcess(), >> but remove it only in Qt 7. This means we get the benefit of cleaner >> API over time, without the feature freeze exception and potential >> standard library compatibility issue, but makes us keep using >> FORKFD_USE_FORK for other derivation reasons. IMHO that's the safer route. BR, Joerg ___ Development mailing list Development@qt-project.org https://lists.qt-project.org/listinfo/development
Re: [Development] Qt5.15 deprecating & Qt6 removing QProcess::setupChildProcess
Thiago Macieira (17 February 2020 20:35) wrote:
> Sorry, this just occurred to me. This is a request for 5.15 feature freeze
> exception.
>
> Re: https://bugreports.qt.io/browse/QTBUG-17331
Closed: 30-07-2013 03:38, >9 years ago.
> Re: 97645478de3ceffce11f58eab140c4c775e48be5
> ("QProcess: use FFD_USE_FORK when the class is not QProcess itself")
Committed to 5.15, has not appeared in any release yet.
Modifies (only) corelib/io/qprocess_unix.cpp, only internal to
QProcessPrivate::startProcess(); no API change.
> The bug report is complaining that the use of fork() instead of
> vfork() on some systems with very low memory left (and no overcommit)
> can cause fork() to fail. The commit I pointed to is a workaround I
> had to add after adding pidfd support for when setupChildProcess() was
> overridden, due to differences in internal glibc locking differences
> between fork() and clone().
>
> Note also that setupChildProcess() does not work on Windows, where
> there's no fork() [trust me, I spent a great deal of time researching
> this topic]. Instead, we have a callback with
> setCreateProcessArgumentsModifier(). It also didn't use to work on
> QNX, where we used posix_spawn(), though that's gone by now.
>
> I don't think we can remove the ability to run user code in the child
> process. But can we remove the virtual? Can we just have a callback
> like on Windows?
I take it that would involve an API change.
I see QProcess has differences in API between MS and Unix.
Reducing that inconsistency would be good.
We would presumably want to subsequently rationalise the MS and Unix
APIs to have a common form, instead of having naked MS types in the API
for one and something else for the other. That would mean deprecating
the existing MS-specific API in favour of a cross-platform API at some
point. Please estimate which versions of Qt the different stages of
that unification would happen in, given that you start in 5.15 (with an
FF exception) vs if you have to wait for 6.
> Pros:
> - users won't have to derive from QProcess to run code in the child
> process (cleaner API)
> - less fragile in detecting when the user wants to enable
> child-running code versus having overridden for other reasons
I like more flexibility for users and more robustness.
> Cons:
> - breaks feature freeze, adds SIC for Qt 6
Please clarify: if we include some new API in 5.15 to support this, what
will be the SiC change in Qt 6 ?
> - of the 6 times I can find of QProcess being derived from in Qt & Qt
> Creator, 5 are to override setupChildProcess anyway and the last one
> is in tst_QProcess
Does anyone have sources from outside Qt project that are currently
forced to derive from QProcess for the sake of this ? If so, please try
to work out how Thiago's change would affect you and give the rest of us
a summary.
> - use of std::function in Qt 5 on Unix may not be
> acceptable (it was for Windows because there are only two compilers)
>
> Alternative: add the API to Qt 6 and deprecate setupChildProcess(),
> but remove it only in Qt 7. This means we get the benefit of cleaner
> API over time, without the feature freeze exception and potential
> standard library compatibility issue, but makes us keep using
> FORKFD_USE_FORK for other derivation reasons.
>
> Opinions?
I know that I do not know enough to have a clear opinion.
Eddy.
___
Development mailing list
Development@qt-project.org
https://lists.qt-project.org/listinfo/development
