Re: 1.14.4 release
On Mon, Mar 11, 2024 at 8:55 AM Stefan Sperling wrote: > > On Sun, Mar 10, 2024 at 07:59:38PM -0400, Nathan Hartman wrote: > > There is at least a week before I can begin working on it, so there is > > time for the dev community to respond. > > I will be around to help with testing/signing. Thanks for picking this up! Likewise. +1 -- Johan
Re: Assert-Anweisung schlug fehl (! svn_path_is_url(relative))
On Tue, Feb 6, 2024 at 10:24 PM Jürgen Loh wrote: > > Am 06.02.2024 um 12:00 schrieb Jürgen Loh: > > This evening I can try if TourtoiseSVN fails on the clients, too. > > I did this test. The problem is not related to SUBST, it happens the > same on a network share. > > The situation is: > > The SVN working copy is on a Windows server, store at C:\User\Data: > > Revert fails on the same server on drive G:\ that is a SUBST to > C:\User\Data. > Revert fails on a client that linked G:\ to a network share on the > Windows server that points to C:\User\Data. > But: Revert WORKS on the server if it is executed on C:\User\Data directly. > > I still don't know if it's TortoiseSVN or SVN related. I don't know how > to test it with the SVN command line client directly. You can install the commandline client that comes with TortoiseSVN by checking the "command line tools" option in the installer. If you have those installed, you can simply open a cmd window, go to G:\ or C:\User\Data, and run 'svn revert' (or 'svn revert -R' if you want to perform the revert operation recursively -- be aware that you'll throw away local uncommitted changes this way, but I assume you know what revert does). I'd like to point out that sharing a working copy this way, allowing multiple users to access the same working copy from different machines (especially if they might use it concurrently), is quite dangerous, and you risk corrupting your working copy (see [1] from TortoiseSVN's FAQ). I'd strongly advise against it. The normal usage pattern is that every user / machine checks out its own working copy from the central repository, runs 'svn update' regularly to keep it updated, and works from their own copy. [1] https://tortoisesvn.net/faq.html#wconshare -- Johan
Re: Switching from SHA1 to a checksum type without known collisions in 1.15 working copy format
On Fri, Jan 12, 2024 at 12:37 PM Daniel Shahaf wrote: ... > Procedurally, the long hiatus is counterproductive. Neither kfogel nor > I had the context in our heads, and the cache misses took their toll in > tuits and in wallclock time. Furthermore, I have less spare time for > dev@ discussions than I did when I cast the veto (= a year ago next > Saturday). Going forward it might be preferable for threads not to > hibernate. I agree, but obviously the hibernation is not some deliberate action by anyone. It's just that most of us here have less spare time for dev@ discussions (and for SVN development) than before. Especially for such complex matters, and especially when people feel there are walking into a minefield. There are only a few active devs left, and tuits are running low ... ... > That being the case, I have considered whether merging the feature > branch outweighs letting dev@ take a not-only-/pro forma/ role in > design discussions. I am of the opinion that it does not, and > therefore I reäfirrm the veto. It has become more clear to me (I was only following tangentially) that your veto is focused on the development methodology and the lack of design discussion. Is that a valid reason for a veto? We are low on resources, someone still finds time to make some progress, no one blocks it on technical grounds, and then someone vetoes it because we don't have enough resources? That puts us pretty much in deadlock, because we are too low on resources. Or maybe I misunderstand? To be clear: I appreciate your input, Daniel, and your insistence on a more thorough design discussion. I assume it's coming from a genuine concern that we formulate problems well, and think hard about possible solutions (focusing on the precise problem we are trying to solve). But at the end of the day, if that design discussion doesn't happen (or not enough to your satisfaction anyway), is that grounds for a veto? For me it's a tough call, because on the one hand you have a point, but on the other hand ... you're blocking _some_ progress because the process behind it is not perfect (which is hard to do with the 3.25 tuits we have left). > P.S. Could that BRANCH-README please state what's the problem the branch > means to solve, i.e., the goal / acceptance test? "Make it possible to > «svn add» SHA-1 collisions"? I agree that would be a good step. I too find it a bit unclear what problem we're actually trying to solve, apart from a vague feeling that SHA-1 will become more and more broken over time, and that this will cause fatal injury to SVN (in its WC, protocol, dump format, or repository). And perhaps the fact that security auditors are becoming more and more triggered by seeing SHA-1 (even if they don't understand the way it is used and its ramifications). Making it possible to 'svn add' SHA-1 collisions is not it, I think. -- Johan
Re: Changing the permission checks in libsvn_subr/io.c
On Sun, Jan 7, 2024 at 2:46 AM Vincent Lefevre wrote: > On 2024-01-05 11:29:16 +0100, Daniel Sahlberg wrote: > > Den fre 5 jan. 2024 kl 10:51 skrev Johan Corveleyn : > > > > > On Fri, Jan 5, 2024 at 8:46 AM Daniel Sahlberg > > > wrote: > > > ... > > > > Since the file doesn't have svn:needs-lock it should be RW [and the > > > Reverted message comes from Subversion trying to restore the W flag ...] > > > > > > Should it? Intuitively I'd say: since the file doesn't have > > > svn:needs-lock Subversion shouldn't be looking at R or RW. Why should > > > we make a file RW? Can't the user make a file readonly just locally, > > > and expect Subversion not to care? > > > > > > Or is "making a file readonly" a committable local change? Will it > > > show up on 'svn st' and can it be committed as some change that can be > > > transferred to another working copy? > > > > > > I understand that svn:needs-lock adds extra handling of the readonly > > > status of files, but without that property? > > > > All good questions, and I probably agree with you: if svn:needs-lock isn't > > set then Subversion could just ignore the R/RW status. > > I also agree. I never use svn:needs-lock, and I want to be able to > set some files in my working copy to read-only, in order to make sure > that I won't modify them by mistake. > > > But any change here would change previous behaviour so it would need > > a solid consensus. > > In any case, the current behavior of Subversion is inconsistent. > "svn revert" is not documented as the command to "fix" the permissions. > > > If the check is removed for files that doesn't svn:needs-lock, then we > > might have to add code to restore RW status if svn:needs-lock is removed. > > > > Making a file readonly is currently not a committable change, didn't check > > 'svn st' but it will be reverted by 'svn revert' and it will not be > > transferred to another WC. It can only be committed indirectly via > > svn:needs-lock. > > > > Any discussion regarding svn:needs-lock probably also have to consider > > svn:executable, since it is handled similarly (except on WIN32 and OS2, > > where the concept of executable doesn't exists). > > > > I havn't completely made up my mind, but I think I favour keeping the > > current behaviour: R/RW status in indicated by the svn:needs-lock property > > and you shouldn't change R/RW manually within a WC. > > Then this should be documented. > > But "svn st" should detect and report incorrect permissions, and > "svn up" should fix them, just like what happens when a file has > been removed with just "rm" instead of "svn delete". Interesting discussion. I agree it should at least be documented, and perhaps be made a bit more clear from the output of 'revert' (but not sure how far we can go without breaking compat). Changing the current behavior is probably a more risky move, given the maturity of SVN and backwards compatibility etc. BTW, I did some more digging because the problem looked familiar, and apparently there was a discussion on users@ in 2016 [1] in which I participated :). It was reported with a Windows client. I ended up filing an issue which contains some interesting background information by Bert [2]. A quote that stood out to me just now when I was rereading it: "For WC-NG we explicitly made 'svn revert' always revert your working copy match how it would be after a clean checkout. (I think philipm worked on this at the time). And he did quite a bit more work to ensure that we always provide a notification when we change the working copy. (Older versions could change your working copy, but not notify you about this) ... I think it might have been better to use a specific notify for 'fixing read-only-ness' during revert, but I'm not sure if this is really worth all the backwards compatibility breakage to apply this three versions after this behavior was introduced." BTW, I know about a similar issue of 'spurious revert notifications', with mismatching timestamps (lastmod-time different from svn metadata, a condition that is normally fixed by 'svn cleanup'): if you "touch" a file in your WC so it has a different timestamp from the metadata, it will be notified when running 'svn revert' (and I believe the metadata is adjusted in this case). So that's another source of spurious revert notifications. I thought I had discussed this on one of our mailinglists, but I can't find it. The fact that revert fixes timestamps is mentioned in passing in issue SVN-4162. Perhaps both types of "adjustments" by revert should get a specific notification (if any -- for the timestamp fixup case I can live with simply not notifying, it's an adjustment in metadata, not on-disk file after all). [1] https://svn.haxx.se/users/archive-2016-03/0004.shtml [2] https://issues.apache.org/jira/browse/SVN-4637 (Revert affects unchanged files with changed permissions) [3] https://issues.apache.org/jira/browse/SVN-4162 (make svn status fix timestamp mismatches in meta-data) -- Johan
Re: Changing the permission checks in libsvn_subr/io.c
On Fri, Jan 5, 2024 at 8:46 AM Daniel Sahlberg wrote: ... > Since the file doesn't have svn:needs-lock it should be RW [and the Reverted > message comes from Subversion trying to restore the W flag ...] Should it? Intuitively I'd say: since the file doesn't have svn:needs-lock Subversion shouldn't be looking at R or RW. Why should we make a file RW? Can't the user make a file readonly just locally, and expect Subversion not to care? Or is "making a file readonly" a committable local change? Will it show up on 'svn st' and can it be committed as some change that can be transferred to another working copy? I understand that svn:needs-lock adds extra handling of the readonly status of files, but without that property? -- Johan
Re: Subversion 1.14.3 up for testing/signing
On Sat, Dec 9, 2023 at 4:50 PM Nathan Hartman wrote: > > The 1.14.3 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. > > Thanks! Summary --- +1 to release (Windows) Platform Windows 10 x64 (Version 1903) Microsoft Visual Studio 2019 Community Edition (Version 16.5.3) Verified Signature and sha512 for subversion-1.14.3.zip. Contents of subversion-1.14.3.zip are identical to tags/1.14.3, and to branches/1.14.x@1914484 (except for expected differences in svn_version.h and svnpubsub, svnwcsub and nominate.pl (symlinks vs. file contents), and generated files). Tested -- [ Release build x64 ] x [ fsfs ] x [ file | svn | http ] javahl swig-python Results --- All tests pass. Dependencies httpd 2.4.58 (apr 1.7.4, apr-util 1.6.3, pcre 8.45, expat 2.2.9, openssl 3.0.12) apr 1.7.4 apr-util 1.6.3 openssl 3.0.12 serf 1.3.10 sqlite 3.44.2.0 zlib 1.3 python 3.9.1 py3c 1.4 swig 4.1.1 (bundled lz4 1.7.5) (bundled utf8proc 2.1.0) Manually applied sqlite fix for incomplete #ifdef checks related to the definition of sqlite3PagerWalSystemErrno (https://sqlite.org/src/info/7374c2342e66b352) Other tools --- perl 5.38.0.1 (Strawberry Perl) python 3.9.1 Oracle JDK 21.0.1 junit 4.13.2 Signature - subversion-1.14.3.zip: -BEGIN PGP SIGNATURE- iQIzBAABCgAdFiEEiqLBDuqtRPlpcnrqtZzm1gEMiq0FAmWGGiwACgkQtZzm1gEM iq1nTBAArXjdcjZwEkAJ+naXAEbpE9jdsFCA6VpRtjcwsWS15hyXk+PpmNFeARs6 +CUTSZA4JiUp68tMYNOuto9lpK6tMpr+XMKldz/k+mUWUOzRG6ONTriEkB1iH3/x jocYw74F1OsjeHsUyFSMhridXRDOIDQw8B/Zmh4lMjTar3jCrTkG2RjofwiRPj4f MAFlyLTLSsdZvhflUkuRWMdg3yBEg+4qukzmnEijZpnXLhGEp6k7rD7jiiIJHFtr Zq6nG2oOkf+eXZcszy0EIw8297FC4yRJdaDEkWzXlV9hyqEwfBpGeZM3dkv6T13f RxDQ+ctNN9EgNjEI1RXwBhFLSvodWVT4ulmwh6bNh63NTSI/6xBx0mpBn2Kl21sL dSlh+qXMBa9Kuguxmipymrmh83N44NjOJmI1tq3iPdNCy+BZxO3GhdTE/oHH6X8l ppvA5SW5fZAMKFHafgXcTkqqn2xyjxZJ2aiZcfiA9nOjxtfgEYwFnAbYZ2lfbcls lh9mI3XMa6OSV1hEJSJ6jlvjftVv9XhwdUEPs6JM7gGiAuZ7YT/HFc5QZXJUJ6EW V3CHGYulctHKwWj/FDzmE9W2OmrCezSPLbMizKARHo5OcosF9VeUq2V6s4W7+5MR zS6U4LDxFPdE6cMYQUjuR5mnhL3XKoWw5bwAve93710ld3/SoLo= =0Ogy -END PGP SIGNATURE- -- Johan
Re: Subversion 1.14.3 up for testing/signing
On Fri, Dec 22, 2023 at 7:46 AM Daniel Sahlberg wrote: > Den fre 22 dec. 2023 kl 02:49 skrev Johan Corveleyn : ... >> Then, regardless of the above error, when I start building with >> msbuild, I run into following error: >> [[[ >> C:\research\svn\dev\deps64\src\sqlite-amalgamation-3.44.2.0\sqlite3.c(34597,42): >> error C4013: 'sqlite3PagerWalSystemErrno' undefined; assuming extern >> returning >> int >> [C:\research\svn\dev\subversion-1.14.3\build\win32\vcnet-vcproj\libsvn_subr.vcxproj] >> ]]] > > > The declaration is guarded by: > #if defined(SQLITE_USE_SEH) && !defined(SQLITE_OMIT_WAL) > > While the actual use seems to be guarded by > #ifdef SQLITE_USE_SEH > > In our code we have > $ grep -r SQLITE_OMIT_WAL > subversion/libsvn_subr/sqlite3wrapper.c:# define SQLITE_OMIT_WAL 1 > > I assume we trigger some bug in SQLite but I don't have time to dig into the > SQLite source code. It's not the first time we've hit errors with OMIT_WAL Googling around I saw that you already reported this issue to sqlite [1], and it has since been fixed in their repository [2]. Thanks for that! After manually applying that patch here the problem is gone, so I was able to continue with sqlite 3.44.2. I went on to fix a couple of linking issues locally (possibly I'm building some dependencies in other ways than the SVN buildscripts on Windows expect them -- or our buildscripts simply are not adapted to recent changes in apr, apr-util and openssl, not sure -- the path of least resistance was copying some *.dll and *.lib files to where our scripts look for them). Now everything has been built successfully, and I'm running tests ... I hope to finish up and commit my signature later tonight or tomorrow morning. I'll send a transcript of my build steps in a new thread later. [1] https://sqlite.org/forum/info/9819032aac [2] https://sqlite.org/src/info/7374c2342e66b352 -- Johan
Re: Subversion 1.14.3 up for testing/signing
On Fri, Dec 22, 2023 at 3:07 AM Yasuhito FUTATSUKI wrote:
>
> Hi,
>
> On 2023/12/22 10:49, Johan Corveleyn wrote:
...
> > Finally, did some tweaks to get gen_win_dependencies.py to run, but
> > then ran into following error:
> > [[[
> > python gen-make.py --release
> > --with-swig=C:\research\swigwin-4.1.1 --with-py3c=C:\research\py3c-1.4
> > --with-junit=C:\research\svn\dev\deps64\src\junit-4.13.2\junit.jar
> > --with-jdk="C:\Program Files\Java\jdk-21" --with-httpd=C:\Apache2.4.58
> > --with-serf=C:\research\svn\dev\deps64\src\serf-1.3.10
> > --with-openssl=C:\research\svn\dev\deps64\src\openssl-3.0.12
> > --with-sqlite=C:\research\svn\dev\deps64\src\sqlite-amalgamation-3.44.2.0
> > --with-zlib=C:\research\svn\dev\deps64\src\zlib-1.3
> > --vsnet-version=2019 -t vcproj 2>&1 | tee log.gen-make
> > C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:233:
> > SyntaxWarning: invalid escape sequence '\.'
> > if val == '2002' or re.match('^7(\.\d+)?$', val):
> > C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:238:
> > SyntaxWarning: invalid escape sequence '\.'
> > elif val == '2003' or re.match('^8(\.\d+)?$', val):
> > C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:243:
> > SyntaxWarning: invalid escape sequence '\.'
> > elif val == '2005' or re.match('^9(\.\d+)?$', val):
> > C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:248:
> > SyntaxWarning: invalid escape sequence '\.'
> > elif val == '2008' or re.match('^10(\.\d+)?$', val):
> > C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:283:
> > SyntaxWarning: invalid escape sequence '\d'
> > elif re.match('^20\d+$', val):
> > C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:290:
> > SyntaxWarning: invalid escape sequence '\d'
> > elif re.match('^1\d+$', val):
> > C:\research\svn\dev\subversion-1.14.3\build\transform_sql.py:53:
> > SyntaxWarning: invalid escape sequence '\('
> > re_statement = re.compile('-- *STMT_([A-Z_0-9]+)( +\(([^\)]*)\))?')
> > Generating for Visual Studio 2019
> > ]]]
> >
> > That is using Python 3.12.1.
> >
> > When downgrading to Python 3.9 those errors are gone. Just wanted to
> > let you guys know ...
>
> Should we backport r1912632?
Ah, thanks. I missed that. Thanks for pointing it out.
Agreed that we should probably backport this to 1.14.x (but as Daniel
said: not a blocker for this release).
--
Johan
Re: Subversion 1.14.3 up for testing/signing
On Tue, Dec 19, 2023 at 1:30 PM Johan Corveleyn wrote:
>
> On Mon, Dec 18, 2023 at 11:13 PM Nathan Hartman
> wrote:
> >
> > On Sat, Dec 9, 2023 at 10:50 AM Nathan Hartman
> > wrote:
> > >
> > > The 1.14.3 release artifacts are now available for testing/signing.
> > > Please get the tarballs from
> > > https://dist.apache.org/repos/dist/dev/subversion
> > > and add your signatures there.
> > >
> > > Thanks!
> >
> >
> > Hi all,
> >
> > Just to help plan ahead a little bit: Who else is planning to
> > test/sign the 1.14.3 tarballs, and approximately how much time do you
> > need?
> >
> > (If it's possible to be done with testing/signing toward the
> > middle/end of this week, I will have a good opportunity to finish up
> > the release work, but please let me know...)
>
> I still plan to go for it (on Windows 10). Not sure if middle of this
> week (i.e. Wednesday night or so) is still realistic, but I'll try. No
> guarantees though, so if you have enough sigs feel free to go ahead.
>
> I started with some initial steps last weekend, but got stranded along
> the way. Basically I realized that OpenSSL 1.1.1 is totally EOL, and I
> should really go for OpenSSL 3 now, which forces me to rebuild serf
> and httpd (fortunately, others have already done work on serf 1.3.10
> to make it work with OpenSSL 3, so I hope that will work out smoothly
> -- as for httpd, that's always a big adventure on Windows). Oh and I'd
> like the latest version of APR 1.7 as well, which includes a relevant
> fix for Windows junctions / subst / something. So far, I've just
> collected everything I need, but have not run a single buildscript
> yet. To be continued ...
Still working on it.
Succesfully built zlib-1.3, openssl-3.0.12, apr-1.7.4, apr-util-1.6.3,
httpd-2.4.58 (and pcre-8.45 and expat-2.2.9) and serf 1.3.10.
Finally, did some tweaks to get gen_win_dependencies.py to run, but
then ran into following error:
[[[
python gen-make.py --release
--with-swig=C:\research\swigwin-4.1.1 --with-py3c=C:\research\py3c-1.4
--with-junit=C:\research\svn\dev\deps64\src\junit-4.13.2\junit.jar
--with-jdk="C:\Program Files\Java\jdk-21" --with-httpd=C:\Apache2.4.58
--with-serf=C:\research\svn\dev\deps64\src\serf-1.3.10
--with-openssl=C:\research\svn\dev\deps64\src\openssl-3.0.12
--with-sqlite=C:\research\svn\dev\deps64\src\sqlite-amalgamation-3.44.2.0
--with-zlib=C:\research\svn\dev\deps64\src\zlib-1.3
--vsnet-version=2019 -t vcproj 2>&1 | tee log.gen-make
C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:233:
SyntaxWarning: invalid escape sequence '\.'
if val == '2002' or re.match('^7(\.\d+)?$', val):
C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:238:
SyntaxWarning: invalid escape sequence '\.'
elif val == '2003' or re.match('^8(\.\d+)?$', val):
C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:243:
SyntaxWarning: invalid escape sequence '\.'
elif val == '2005' or re.match('^9(\.\d+)?$', val):
C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:248:
SyntaxWarning: invalid escape sequence '\.'
elif val == '2008' or re.match('^10(\.\d+)?$', val):
C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:283:
SyntaxWarning: invalid escape sequence '\d'
elif re.match('^20\d+$', val):
C:\research\svn\dev\subversion-1.14.3\build\generator\gen_win_dependencies.py:290:
SyntaxWarning: invalid escape sequence '\d'
elif re.match('^1\d+$', val):
C:\research\svn\dev\subversion-1.14.3\build\transform_sql.py:53:
SyntaxWarning: invalid escape sequence '\('
re_statement = re.compile('-- *STMT_([A-Z_0-9]+)( +\(([^\)]*)\))?')
Generating for Visual Studio 2019
]]]
That is using Python 3.12.1.
When downgrading to Python 3.9 those errors are gone. Just wanted to
let you guys know ...
Then, regardless of the above error, when I start building with
msbuild, I run into following error:
[[[
C:\research\svn\dev\deps64\src\sqlite-amalgamation-3.44.2.0\sqlite3.c(34597,42):
error C4013: 'sqlite3PagerWalSystemErrno' undefined; assuming extern
returning
int
[C:\research\svn\dev\subversion-1.14.3\build\win32\vcnet-vcproj\libsvn_subr.vcxproj]
]]]
I ran out of time for tonight, will continue tomorrow. If anybody
knows what I should do about the above error, let me know :-).
Otherwise I'll try downgrading sqlite to 3.41.2 (the version Evgeny
Kotkov used a couple of days ago).
--
Johan
Re: Subversion 1.14.3 up for testing/signing
On Mon, Dec 18, 2023 at 11:13 PM Nathan Hartman wrote: > > On Sat, Dec 9, 2023 at 10:50 AM Nathan Hartman > wrote: > > > > The 1.14.3 release artifacts are now available for testing/signing. > > Please get the tarballs from > > https://dist.apache.org/repos/dist/dev/subversion > > and add your signatures there. > > > > Thanks! > > > Hi all, > > Just to help plan ahead a little bit: Who else is planning to > test/sign the 1.14.3 tarballs, and approximately how much time do you > need? > > (If it's possible to be done with testing/signing toward the > middle/end of this week, I will have a good opportunity to finish up > the release work, but please let me know...) I still plan to go for it (on Windows 10). Not sure if middle of this week (i.e. Wednesday night or so) is still realistic, but I'll try. No guarantees though, so if you have enough sigs feel free to go ahead. I started with some initial steps last weekend, but got stranded along the way. Basically I realized that OpenSSL 1.1.1 is totally EOL, and I should really go for OpenSSL 3 now, which forces me to rebuild serf and httpd (fortunately, others have already done work on serf 1.3.10 to make it work with OpenSSL 3, so I hope that will work out smoothly -- as for httpd, that's always a big adventure on Windows). Oh and I'd like the latest version of APR 1.7 as well, which includes a relevant fix for Windows junctions / subst / something. So far, I've just collected everything I need, but have not run a single buildscript yet. To be continued ... -- Johan
Re: 1.14.3 release planning
Sounds like a plan :). I'm currently swamped with $dayjob myself, until half of next week at least. After which I'll try to join the effort (I'll first need to blow the dust off my old svn-dev environment, so don't expect me to be quick, but I'll try). -- Johan On Tue, Dec 5, 2023 at 11:25 AM Daniel Sahlberg wrote: > > Hi > > Sounds great! Good work! > > I’m working on preparations for signing on my end, will try to get everything > going on Windows and Ubuntu 23.10 this time. > > Kind regards > Daniel > Daniel > > mån 4 dec. 2023 kl. 18:34 skrev Nathan Hartman : >> >> Hi all, >> >> Just a heads up, I plan to roll tarballs this week. >> >> Hopefully all goes well and we'll be able to make the release soon... >> >> Cheers, >> Nathan
Re: Is there a write opposite to "svn cat"?
>>> The documentation say: >>> [[[ >>> put SRC-FILE URL : add or modify file URL with contents copied from >>>SRC-FILE (use "-" to read from standard input) >>> ]]] Just chiming in here from the sideline, without ability to test right now, but: has anyone tried to use a full "-" (double quote, dash, double quote) as argument? As in: svnmucc put "-" URL Maybe that's what that help text is actually trying to say. -- Johan
Re: New release
On Thu, Nov 9, 2023 at 6:08 PM Nathan Hartman wrote: > > On Fri, Nov 3, 2023 at 12:00 AM Nathan Hartman > wrote: > > Previously I mentioned I plan to RM for the upcoming 1.14.3 release. > > This being my first time, I need to solve some issues first. > > > > One of these, which has been a stumbling block for me since the > > beginning, is getting the JavaHL bindings to build and test > > successfully. > > > > I have made progress on this, but not a complete breakthrough yet. > > First, thanks to Stefan, Daniel, and Mark for your input. > > I haven't solved my issue with the JavaHL tests yet. I have (mostly) > written a detailed reply, where I respond to each question and > suggestion. I'll send it soon, but first, while I was writing it, I > thought of several specific additional things I want to check. So, > rather than send a reply full of loose ends, I'll investigate those > items (probably tonight) and I'll respond then -- wish me luck, and > hopefully I'll be able to report success! > > Nathan Hi Nathan, Sorry I haven't helped so far ... I should be able to say something useful here, since I'm a java dev in my dayjob. But I have been drowning a bit in that same dayjob lately, so I can barely find the time to scan these mails, and then forget about them because my attention is needed elsewhere. Anyway, I'll try to find some more time to take a closer look at the errors you get. Just as a first shot: if there is no RunTests.class in the location where it should be (one of the directories in the classpath), then it probably has not been compiled (or it has not been put in the right location). My first guess would also be, like Daniel mentioned, that you'll need the --with-junit option (referring to a junit-X.Y.jar somewhere on your system) and then the build scripts should compile the test classes. But I guess you already tried that, after Daniel's reply, and you're still running into problems. I'll take a closer look when you send your detailed mail then. -- Johan
Re: New release
On Fri, Oct 13, 2023 at 5:35 PM Stefan Sperling wrote: > > On Fri, Oct 13, 2023 at 08:43:59AM +0200, Daniel Sahlberg wrote: > > Hi, > > > > There are quite a number of improvements waiting to be released. Can we > > muster the energy to do a new release? > > > > In trunk there are a lot of changes that warrant a 1.15, but before doing > > that I think we should also go back to the discussion of changing the / > > adding a checksum algorithm in the WC. That deserves a separate thread and > > I'm half way through summarising the previous discussions so I'd like to > > hold back 1.15 for the moment. > > > > Still in 1.14 there have been a number of bugfixes that might be good to > > get released. Maybe doing 1.14.3 first could set us up to do 1.15 a little > > later? > > The first problem to solve before the ball starts rolling would be > finding a release manager. I don't have enough spare time to play RM > this time around but I would support the RM as far as my time allows for. > > Doing 1.14.3 first sounds like a good plan. This would help potential > new releases managers to get bootstrapped into the process. A major > release tends to involve a little bit more effort because some new problems > may only show up on specific operating system platforms during -rc testing. +1 -- Johan
Fwd: Public funding for contributions available
Maybe interesting for some people here on dev@subversion? The switch from funded development to almost-all-volunteer work has been hard for our little community, and I always thought some of the originally-funded-devs might have wanted to continue some work here, if only there would be a way to organize funding ... And if there are already other avenues, maybe this just provides some additional options. -- Johan -- Forwarded message - From: Isabel Drost-Fromm Date: Thu, Jun 8, 2023 at 11:14 AM Subject: Public funding for contributions available To: Hi, tl;dr: Want to contribute to Open Source (including projects here at the ASF) and have that time publicly funded? Head over to: https://sovereigntechfund.de/en/challenges/ >From the title page: "The Sovereign Tech Fund is looking for developers who use open source and want to contribute back to it. [...] In three challenges, participants can work on contributing back to open source for up to eight months with a budget of up to €300,000 per round. Many more people use open source software every day than contribute to it. It’s time to give back and invest in this ecosystem, to increase its security and sustainability, and to create a digital world that we collectively shape. The Sovereign Tech Fund invests in open digital infrastructure. For us, this means fundamental technologies that enable the creation of other software. These components – like libraries and open standards – are openly accessible, trustworthy and can be used freely." >From the description of the projects they want to fund I see a lot of overlap with our projects here at the ASF. Feel free to share this information publicly and widely! Warm regards, Isabel
Re: Plaintext cache release notes (was: svn commit: r1909352 - /subversion/trunk/CHANGES)
On Tue, Apr 25, 2023 at 5:00 AM Nathan Hartman wrote: > > On Mon, Apr 24, 2023 at 12:38 PM Daniel Sahlberg > wrote: > > > > Den mån 24 apr. 2023 kl 18:11 skrev Nathan Hartman > > : > (snip) > >> Sometime in the next few days, I'll draft some text for the 1.15.x > >> release notes about this change (unless you or someone else wants to > >> do it or has already started; in that case, just let me know)... Also > >> the FAQ entry might need an update; I'll look at that at the same > >> time. > > > > > > Great! Thanks for taking the time to do this! > > Here's a first draft, obviously without HTML markup... > > Whatever this ultimately morphs into will go in the 1.15 release notes. > > Normally, I would not recommend to write a dissertation in the release > notes, but I think there is a lot of room for confusion and > misunderstandings, so perhaps it is better to be a little verbose here > and present the brief history of how we got here and why we're making > this change. > > Feedback welcome! > > [[[ > > # Plaintext credential cache is supported by default on Unix-like systems > > Subversion supports several credential caches to prevent re-typing > usernames and passwords repeatedly. Which credential cache(s) are used > depends on the operating system, compile-time options, and the user's > runtime configuration. On Windows and macOS, Subversion uses OS > facilities to save passwords in encrypted form. Unix-like operating > systems do not have a single standard facility to do this; on these > systems, Subversion supports up to four credential caches: GNOME > Keyring, KWallet, GPG Agent, and (as a fallback) the Plaintext cache. > > The rest of this section discusses the Plaintext cache and is > applicable only to Subversion clients running on Unix-like operating > systems. > > In Subversion 1.12 through 1.14, write access to the Plaintext cache > was disabled by default at _compile-time_. Binaries compiled in the > default configuration could not store new plaintext credentials, but > would continue to use any that were already stored. Users and binary > packagers could explicitly enable write access to the Plaintext cache > by compiling Subversion with the --enable-plaintext-password-storage > option to configure. (See r1845377.) > > Unfortunately, this has caused a variety of problems for users, > especially when using the svn client in unattended processes such as > CI systems, or on remote machines through ssh (a GUI password prompt > would display on the remote machine, inaccessible to the ssh user). > Users reported that they had to employ workarounds that caused > passwords to be stored in plaintext anyway, or refused to upgrade > their Subversion installations to these releases. Some binary > packagers built with --enable-plaintext-password-storage while others > didn't, creating inconsistent experiences within the same release > lines. > > Based on the feedback received, Subversion 1.15 inverts the default. > (See r1909351.) Binaries compiled in the default configuration can > once again store new plaintext credentials (after warning and asking > the user). Sites that wish to eliminate this possibility can do one or > both of the following: > > * Compile Subversion with the --disable-plaintext-password-storage > option to configure or install a binary package that was compiled this > way. Be aware that users can circumvent this by compiling or > installing their own Subversion binaries and/or by creating a > plaintext cache manually. > > * Allow encrypted stores like GNOME Keyring and KWallet, but not the > Plaintext cache, by setting store-plaintext-passwords = no in > Subversion's run-time config settings. See the per user files at > ~/.subversion/config and ~/.subversion/servers, and the systemwide > files at /etc/subversion/config and /etc/subversion/servers. > > For more on plaintext credentials, see the FAQ entry: > https://subversion.apache.org/faq.html#plaintext-passwords > > ]]] Great. Well written, looks good to me. -- Johan
Re: svn commit: r1909352 - /subversion/trunk/CHANGES
On Sat, Apr 22, 2023 at 6:25 PM wrote: > > Author: dsahlberg > Date: Sat Apr 22 16:25:30 2023 > New Revision: 1909352 > > URL: http://svn.apache.org/viewvc?rev=1909352=rev > Log: > * CHANGES: Document r1909351 ... > + - Other tool improvements and bugfixes: > +* Storing passwords in plain text on disk is again enabled by default > (r1909351) I would suggest rephrasing this a bit, to make it clear that we simply re-enabled the *possibility* of storing passwords in plain text (on Unix, if the runtime configuration allows it and no better password stores are available). Obviously that's a bit long, but I can't come up with a good concrete suggestion right now, sorry :-). But as it reads there it could be interpreted as "wow? what?". -- Johan
Re: [VOTE] Reverting r1845377 (Was: [PROPOSAL] Allow plaintext passwords again.)
On Sun, Apr 16, 2023 at 11:19 PM Daniel Sahlberg
wrote:
>
> The dicussion died again, but this time I intend make sure we complete it
> once and for all. I've marked the subject as VOTE to hopefully get some
> attention, although I believe votes have already been cast.
Thanks for picking it up once again :-). Let's hope we can land this now ...
> In my mind, it seems we have consensus to revert r1845377 (+1 from Nathan
> Hartman, Evgeny Kotkov, Johan Corveleyn, myself, I'm also considering Karl
> Fogel to have voted for this by making the initial proposition and
> volunteering to do it. No objections).
Indeed, +1 from me.
> Then we have a two other suggestions:
>
> * Changing the default value of store-plaintext-passwords
> Option 1 - set the default value of store-plaintext-passwords to no. For a
> non-edited servers config file, this will put the behaviour of 1.15 in line
> with 1.12-1.14.
> Option 2 - keep store-plaintext-passwords = ask. For a non-edited servers
> config file, the behaviour of 1.15 will be in line with pre-1.12.
>
> I have previously expressed support (but no formal vote, and I will not stand
> in the way of consensus) for option 1, since I was under the impression that
> it was a longterm goal go disable the plaintext password store (I have
> previously commented on how quickly r1845377 was discussed and implemented).
> Nathan has argued that it might give a bad user experience if credentials are
> not stored even though plaintext storage is enabled in the build options. We
> can improve on this by updating the svn --version output to explicitly say if
> plaintext cache is build but runtime disabled.
>
> If I'm counting correctly, Nathan, Evgeny and Johan has expressed +1 for
> option 2.
>
> Considering this, I conclude we have consensus for option 2.
>
>
> * Changing handling of already stored plaintext passwords
> This was discussed in 2022 and there were some suggestions that Subversion
> should not even use plaintext passwords even if they were found in the
> plaintext password store (initially suggested by Mark Phippard as a way to
> soften the impact of reverting r1845377). I'm proposing we choose amoung one
> of the following three options:
>
> Option 1 - do nothing. Keep using the stored passwords.
> Option 2 - add a new runtime config option dont-use-plaintext-passwords
> [default no] global overrides local.
> Option 3 - new compile time option to disable reading/using plaintext
> passwords.
Why a new compile-time option for disabling reading? Why not keep it
simple, and disable the reading of plaintext passwords if the
compile-time option that will be re-introduced by reverting r1845377
is supplied (--disable-plaintext-password-storage)? I think that is
what Mark was suggesting, and I still like it because it's simple and
doesn't add more knobs than necessary.
So maybe:
Option 4 - disable reading/using plaintext passwords if compile-time
/storing/ of plaintext-passwords is configured
(--disable-plaintext-password-storage).
>
> Options 2 and 3 should probably imply disabling storing plaintext passwords
> as well. I think they should also warn if the code finds a stored password
> and suggest the user to delete it using svn auth --remove. (It was hinted
> previously that we would disable the code that searches for the files storing
> the password, however the same files also store the username and we read them
> as an apr_hash_t so we don't really have the option to "not read the
> password").
>
> With option 2 or 3 we let a security consious organisation configure their
> system to their liking. I would love to have them, but I can't avoid the
> feeling that it is security theater: As far as I can tell it is not possible
> to avoid that a user can upload their own version of the svn binary and then
> all bets are off anyway. (On Windows and MacOS it is possible to only allow
> execution of signed binaries, and they don't even store the passwords in
> plaintext anyway).
>
> If we go with either 2 or 3, then I'm perfering option 2 since it will allow
> the administrators to set this without compiling their own version (which
> seems to be a major obstacle, considering the reaction to r1845377). I
> believe Karl Fogel and Mark was also leaning towards doing something during
> the discussion in 2022.
>
> Johan seems to believe option 1 is fine ("these additional \"mitigations\"
> are not absolutely required").
>
> In my mind, it is perfectly acceptable to vote +1 on both option 1 AND one of
> option 2/3. I would interpret that as "do nothing in the short term, but do X
> in the long term".
Agreed. So option 1 follows automatically from reverting r1845377; and
option 2, 3
Re: [PROPOSAL] Allow plaintext passwords again.
On Thu, Mar 30, 2023 at 12:15 AM Nathan Hartman wrote: > > On Wed, Mar 29, 2023 at 6:02 PM Evgeny Kotkov > wrote: > > > > Nathan Hartman writes: > > > > > I think a good middle ground is: > > > > > > * Build with --enable-plaintext-password-storage by default; users who > > > want to harden their system can do so, but will need to build their > > > own client. > > > > +1. +1 > > > * Set the default run-time config to store-plaintext-passwords = no > > > (if it isn't already; haven't checked) and instruct users on how to > > > change it. This makes the decision to store in plaintext an explicit > > > one that the user can opt into. (I appreciate that this could be > > > changed without the user's knowledge; perhaps the systemwide config > > > should always take precedence over the user-controlled one for this > > > setting?) > > > > So, apparently, the current default is "ask". > > > > I haven't checked all the details, but I think that defaulting to "ask" > > already makes the user decision explicit and allows it to happen naturally, > > without requiring any additional instructions or knowledge. > > > > If we change the default to "no", this part of the experience could be > > worse, > > because for the end users it might look like the credentials aren't being > > stored for unknown reasons / a bug in the software. > > Ah, this makes sense. In that case, I'm +1 to leave it as "ask" (no change). +1 Basically this would correspond to kfogel's proposal earlier in this thread [1] (and the one most participants agreed with): "I think it's just a matter of reverting r1845377, right? (And updating CHANGES, etc.)" For completeness, I want to quickly summarize two additional suggestions made by Mark Phippard [2][3]: - If plaintext-pwd-caching support is compiled out (by explicitly giving the compile-time flag for this), also stop reading already cached ones. This would render Daniel's python script useless in these environments. It might satisfy some security sensitive people who would regard the script as a way to "circumvent" the plaintext-disablement. It would make the "plaintext-disabling" more of a complete feature. Additionally, it was suggested that svn could warn or erase such legacy plaintext-cached passwords (instead of just ignoring them), as yet another mitigation improvement. - Apply some obfuscation or encryption with a key hidden in the code (or even supplied by a compile time option) to the plaintext passwords. This helps against non-malicious exposure, and makes it a tad harder for simple scripts to extract the password (which might appease some part of our user base). This might break legitimate scripts that explicitly (mis-)use the cached password for other purposes, though we could regard such uses as hacks that we are allowed to break. IMHO these additional "mitigations" are not absolutely required (I mainly would like to see r1845377 reverted), but if some people feel strongly about them ... sure, why not (though in that case we'd need someone to put in the work too). [1] https://lists.apache.org/thread/shzxh04l493qnj8pdt8vl0x4gkjrkvcy [2] https://lists.apache.org/thread/1tfny40nokqf6p6nll30p06t8or2c8hm [3] https://lists.apache.org/thread/p2vn6foj8qz3lfvdl70bs62vg5krcgr7 -- Johan
Re: [PROPOSAL] Allow plaintext passwords again.
On Mon, Jan 24, 2022 at 5:02 PM Mark Phippard wrote: > > On Mon, Jan 24, 2022 at 10:44 AM Daniel Shahaf > wrote: > > > > > >I return to my "two camps" argument. The people that do not want > > > > >plaintext passwords to be cached ... do not want them being > > > > >cached. > > > > > > > > I see what you mean. > > > > > > > > If svn is compiled to not cache passwords, but a legacy cached > > > > password exists on disk for a given repository, should svn not > > > > only not read it but actually warn the user that the cached > > > > password exists? > > > > > > IMO, it is not necessary and if a compiler option disables the code > > > then I would envision we do not even have any code running that is > > > looking for those files to give the warning. > > > > Plaintext passwords are saved in the "password" element of the > > serialized hash in ~/.subversion/auth/svn.simple/. > > > > Those are the same files that cache the username when the username is > > cached without its password. > > > > We can't know whether a file contains a password or not until we have > > opened, read, and parsed it. > > > > So, "not even have any code running that is looking for those files" > > isn't an option (unless we're willing to throw away cached usernames > > even if they were cached without a password) > > > > So, what should we do if we have parsed one of those files and the > > resulting apr_hash_t contains a "password" key? > > > > Ignore it? Erase it (memset() it to zero)? Warn about it? Use it? > > Good points and excellent questions. If we would already be > discovering this file then I suppose we could do something. I would be > fine with just ignoring the cached password but some kind of other > option would also be good. > > > > And for that matter, should there be a configure option that disables > > the --password command-line option? It, too, can be used insecurely > > (see above about filesystem-level encryption). > > Also a good question. A configure option to disable this might be > appreciated by some users. Is this issue on someone's radar? It seems the discussion died out here, and I can't find anything further. Maybe worth taking another look now that we're getting closer to 1.15? We seemed to get stuck "finding consensus on desired behaviour". Various proposals were made, but none got over the "bar of objections", and we ran out of steam. Which leaves us with the status quo, however imperfect it is :-(. (This recently came up in my company, when we were looking at upgrading the svn client on a unix build machine -- oops can't upgrade past SVN 1.12 or so, because of the compiled-out plain-text pwd caching support) For some background (warning long read): https://lists.apache.org/thread/b6g2hx2m3s117wcmno08opl874ons3q8 https://lists.apache.org/thread/shzxh04l493qnj8pdt8vl0x4gkjrkvcy -- Johan
Re: svn st shows missing for root folder
On Wed, Apr 15, 2020 at 10:40 AM Johan Corveleyn wrote: > > On Tue, Nov 19, 2019 at 9:44 PM Evgeny Kotkov > wrote: > > > > Stefan Kueng writes: > > > > > Using svn 1.13.0 on Windows: > > > > > > SUBST G:\ D:\Development\TestWC > > > G: > > > svn st . > > > > > > ! . > > > ? Foo > > > ? src\nonversioned > > > > > > As you can see, the root folder doesn't show the correct status. > > > This showed the correct status with 1.12.x. > > > > On my side, this issue *does not* reproduce with Subversion 1.13.0 built > > against APR 1.6.x, but reproduces with the TortoiseSVN 1.13.1 command-line > > binaries. > > > > I see that TortoiseSVN 1.13 has recently switched to APR 1.7.0, so that may > > be causing the issue: > > > > https://osdn.net/projects/tortoisesvn/scm/svn/commits/28674 > > > > Among the changes in APR 1.7.0, this one could be responsible for the faulty > > behavior, since it changes how apr_stat() works for reparse points: > > > > https://svn.apache.org/r1855950 > > > > > > Regards, > > Evgeny Kotkov > > Somehow, this fell through the cracks, and was not reported to APR as > it should have been. So it came back to this list later, in another > report (unfortunately still not forwarded to APR): > > > https://lists.apache.org/thread.html/4797303a065e9c1091bbf445acaa3121ccb27ac2d661eb0debd1cf40%40%3Cdev.subversion.apache.org%3E > > Anyway, I've forwarded this thread to dev@apr last week, where it is > currently being discussed: > > > https://lists.apache.org/thread.html/r28e478074055436b27b13f7487203448079aea5adfe4207007ad7ea9%40%3Cdev.apr.apache.org%3E > > We know now that the problem isn't specific to the use of "subst", but > actually to all "working copies on drive roots", so also when you > perform a checkout to C:\ for example. > Probably also with "shared folders" over the network, when the > wc-root is the share-root, as was just reported to TSVN here: > > > https://groups.google.com/forum/?utm_medium=email_source=footer#!msg/tortoisesvn/jgHE5_sHfFI/xOpP1Ce3AAAJ > Just closing the loop on this issue: It has taken some time, but this issue (in APR) has been fixed now and backported to the 1.7.x branch in r1904030. It was released last week as part of APR 1.7.1. (I've also left a message on the tortoisesvn-users list about this, in reply to an old thread about this problem) -- Johan
Re: Getting to first release of pristines-on-demand feature (#525).
My thanks also to the courageous people having developed this, and the
gentle souls keeping the ball rolling :-).
About the name:
On Thu, Nov 24, 2022 at 3:57 PM Nathan Hartman wrote:
...
> Previously we got stuck trying to choose the user-facing name of this
> feature and its command line switches.
>
> Currently the CLI switch is --store-pristine={yes|no}.
>
> I'm okay with this, but for completeness I'll mention that earlier in
> the year there was a little bit of push back because pristines, up
> until now, have been an internal implementation detail that users
> needn't concern themselves with. (Except that they double the storage
> space...)
>
> I've been trying to think of something better for months now, and
> here's what I've come up with:
>
> --optimize=storage
> --optimize=network
FWIW, my vote still goes to --store-pristines={yes|no}
I prefer such an explicit option here, rather than vague ones that
could cover many different things. Also, --optimize=X can easily be
interpreted inversely as intended (for instance: when I have an
optimal network, do I use --optimize=network?)
Apart from {yes|no} the feature might grow other option values in the
future ('size-based' or 'text-only', or maybe simply 'auto' if we come
up with a good general strategy that works for 99% of the cases, the
details of which we don't want to burden our users with). We could
even, in some distant future, allow user-defined names that are
specified in ~/.subversion/config by the user (using some syntax where
the user can set configurable size limits or mime-types or whatever).
One other suggestion: not a blocker of course, but a
runtime-config-area default would be nice :-). Users might want to
choose the same option all the time, without having to remember to add
the option to their checkout command.
Something like, in ~/.suversion/config
store-pristines-default={yes|no}
Just my 2 cents of course ...
--
Johan
Re: Subversion 1.10.0 end-of-life
On Thu, Apr 28, 2022 at 9:26 PM Nathan Hartman wrote: > On Thu, Apr 28, 2022 at 3:12 PM Stefan Sperling wrote: >> On Thu, Apr 28, 2022 at 01:29:43PM +, Daniel Shahaf wrote: >> > Stefan Sperling wrote on Thu, 28 Apr 2022 09:55 +00:00: >> > > I think it would be better to have such details spelled out in English >> > > in a manner that is easy to understand for anyone, with illustrating >> > > examples, instead of (or in addition to) mathematical notation that >> > > requires abstract thinking to figure out. >> > >> > I'm unable to interpret this charitably. >> >> Fine, I'll best just shut up then. > > Please, let's not have any of that!! Indeed! Stefan, I always appreciate your input / feedback / opinion / view on things, just as much as I appreciate Daniel's. So please keep it coming, both of you :-). I did not sense any sort of negative undertone or implicit criticism. Just useful feedback. At least that's my interpretation. Anyhow, moving on ... I agree with the direction this is heading, and +1 to Daniel's concrete patch that was posted a bit later (thanks for spelling things out). (But please, please, please ... keep input, feedback, ideas, chiming in, reservations, cheering, ... coming. Always.) -- Johan
Re: Pristines-on-demand=enabled == format 32?
On Thu, Apr 7, 2022 at 6:07 PM Julian Foad wrote:
>
> Johan Corveleyn wrote:
> > Ah, yes, I think that makes #4889 a blocker.
>
> Well, I'm having a hard time deciding what exactly we need and why.
>
> I previously said "it's pretty clear it needs to be uncoupled" but
> actually just now I've dived into it for a couple of hours, coding and
> thinking, and it's not at all clear what this means.
>
> Is it mainly about UI level naming in "checkout" and "upgrade" -- in
> other words, that we would prefer the user to say
>
> "svn checkout --enable-POD525"
>
> instead of (or in addition to) "--compatible-version=1.15"? And we would
> not need to support the combination of "=1.15" without "--enable-POD525"
> (in other words the feature is still coupled to the format in the
> implementation), but require it to be specified explicitly and error out
> if it isn't, in order to set a precedent that that's the option you'll
> also be needing to use in future versions?
>
> ('POD525' used here as a place-holder for the feature name that is to be
> decided.)
>
> If it's that kind of UI-level naming issue, then we probably want to
> also put corresponding entry in "svn info" saying "POD525 enabled?
> yes/no". And anywhere else the idea is exposed in the UI, if anywhere.
>
> And/or, is it that we want to put internal APIs in place that let the
> higher level code layers ask "is POD525 enabled?" and not have to change
> those callers when 1.16/new-wc-format-33 comes around? But I don't see a
> strong need for that. We are not making a strong case for any of this to
> be exposed in public APIs at this time at all and the internal API calls
> can surely be updated as and when needed.
>
> And/or, is it that we really need users to be able to create a format-32
> (v=1.15) WC that doesn't have POD525 enabled? I am pretty sure that is
> not the case.
Sorry for the late reply, but mainly this, yes. Users will not expect
the latest wc format to "force" them into POD525-enabledness. I think
it would be a mistake to hard-couple format-32 to POD525-enabled. And
uncoupling it only when format-33 comes around with another shiny
feature would be highly confusing IMHO.
Upgrading to the latest format is a natural thing to do. People might
just expect it to fix security bugs, or to give them the latest and
greatest features. People won't expect a simple
TortoiseSVN-right-click->upgrade to completely re-arrange their
working copy into POD525-enabled state (which is an optional feature
that won't benefit every possible working copy). If this is coupled I
am willing to bet that TortoiseSVN will have to introduce a warning
popup for the "upgrade" action telling them about POD525, what it will
do to their pristine-store (trying to explain what it even is to most
users), how it might cripple their experience if they are on
high-latency network, etc etc ...
It's different for WC-features that are not optional and that are the
"way forward" for everyone (like WC-NG was ... there was no option).
But when the new format introduces a choice of WC-layout (both of
which are valid and supported into the future), the format upgrade
itself should not make that "new choice" automatically (possibly
causing huge pristine-store reshuffling right then and there), but
keep things the old way for backwards compatibility, IMHO.
So yes, I think format-32 and POD525 should be decoupled in the API
and in the UI, and stored somewhere in the WC storage (in wc.db, as
Evgeny already suggested elsewhere I think).
--
Johan
Re: Pristines-on-demand: fix disabled tests (#4891)
Okay, with --wc-format-version=1.8, I now get these (X)FAILs and XPASSes (fails_wc-format-version1.8.log in attachment): [[[ XFAIL: diff-diff3-test 18: 3-way merge, double add XFAIL: dirent_uri-test 47: test match with RFC 6125 s. 6.4.3 Rule 3 XFAIL: op-depth-test 42: mixed_rev_move [[needs different libsvn_wc entry point]] XFAIL: op-depth-test 56: commit_moved_away_descendant XFAIL: op-depth-test 68: move retract (issue 4336) XFAIL: op-depth-test 69: move/delete file externals (issue 4293) XFAIL: op-depth-test 75: move more than once, revert intermediate XFAIL: op-depth-test 79: del4: delete AAA XFAIL: op-depth-test 80: del4: add AAA XFAIL: op-depth-test 81: del4: replace AAA XFAIL: op-depth-test 83: del4: replace self AAA XFAIL: op-depth-test 85: move4: delete AAA XFAIL: op-depth-test 87: move4: replace AAA XFAIL: op-depth-test 86: move4: add AAA XFAIL: op-depth-test 89: move4: replace self AAA XFAIL: op-depth-test 95: move within mixed move XFAIL: basic_tests.py 9: basic corruption detection on update [[Relies on wc.text_base_path()]] XFAIL: basic_tests.py 63: peg rev resolution on non-existent wc paths XFAIL: blame_tests.py 15: blame -g handles changes from empty mergeinfo XFAIL: changelist_tests.py 5: diff --changelist (wc-wc and repos-wc) XFAIL: commit_tests.py 66: last changed of copied subdir XFAIL: commit_tests.py 74: commit sees tree conflict on unversioned path XFAIL: copy_tests.py 105: copy and move conflicts XFAIL: depth_tests.py 49: deleted & moved items left untouched XFAIL: depth_tests.py 50: unversioned files in excluded directory XFAIL: diff_tests.py 77: diff repo to wc of a copy XFAIL: diff_tests.py 90: diff unversioned files in git format XFAIL: diff_tests.py 92: diff summary repo wc local copy unmodified XFAIL: diff_tests.py 94: diff git format copy XFAIL: export_tests.py 11: export working copy at base revision XFAIL: externals_tests.py 25: update that modifies a file external XFAIL: externals_tests.py 39: file external remap segfaults due to deleted props XFAIL: externals_tests.py 44: move with file externals XFAIL: externals_tests.py 49: file externals versioned obstruction XFAIL: externals_tests.py 68: check file external recorded info XFAIL: log_tests.py 46: log --use-merge-history --search XFAIL: log_tests.py 47: log --use-merge-history --xml XFAIL: merge_automatic_tests.py 16: cherry2_fwd XFAIL: merge_automatic_tests.py 17: cherry3_fwd XFAIL: merge_tests.py 49: avoid repeated merges for cyclic merging XFAIL: merge_tests.py 64: merge target with non inheritable mergeinfo XFAIL: merge_tests.py 114: don't inherit bogus mergeinfo XFAIL: merge_tests.py 115: don't inherit bogus working mergeinfo XFAIL: patch_tests.py 52: hunks that overlap XFAIL: patch_tests.py 78: patching a specific merge XFAIL: patch_tests.py 80: patch empty prop XFAIL: patch_tests.py 81: patch working copy root XFAIL: patch_tests.py 82: patch working copy root XFAIL: pegrev_parse_tests.py 11: add file '.@tau' without pegrev escape [[The error message mentions '@tau' instead of '.@tau']] XFAIL: pegrev_parse_tests.py 23: add file 'E/@tau' without pegrev escape [[The error message mentions 'E@tau' instead of 'E/@tau']] XFAIL: pegrev_parse_tests.py 25: add file 'E/.@tau' without pegrev escape [[The error message mentions 'E@tau' instead of 'E/.@tau']] XFAIL: pegrev_parse_tests.py 28: add file 'E/@' without pegrev escape [[The error message is E29 but should be E125001]] XFAIL: pegrev_parse_tests.py 39: create directory '.@T' without pegrev escape [[The error message mentions '@T' instead of '.@T']] XFAIL: pegrev_parse_tests.py 49: create directory 'E/@T' without pegrev escape [[The error message mentions 'E@T' instead of 'E/@T']] XFAIL: pegrev_parse_tests.py 51: create directory 'E/.@T' without pegrev escape [[The error message mentions 'E@T' instead of 'E/.@T']] XFAIL: pegrev_parse_tests.py 52: create directory 'E/@' without pegrev escape [[Reports error that E exists but should be E125001 for E/@]] XFAIL: pegrev_parse_tests.py 63: remove '.@kappa' without pegrev escape [[The error message mentions '@kappa' instead of '.@kappa']] XFAIL: pegrev_parse_tests.py 77: remove 'B/@beta' without pegrev escape [[The error message mentions 'B@beta' instead of 'B/@beta']] XFAIL: pegrev_parse_tests.py 79: remove 'D/.@delta' without pegrev escape [[The error message mentions 'D@delta' instead of 'D/.@delta']] XFAIL: pegrev_parse_tests.py 80: remove 'B/@' without pegrev escape [[Removes B instead of reporting E125001 for B/@]] XFAIL: pegrev_parse_tests.py 81: remove missing 'E/@' without pegrev escape [[Removes E instead of reporting ENOENT or E125001 for E/@]] XFAIL: pegrev_parse_tests.py 82: remove missing '@/@' without pegrev escape [[Removes @ instead of reporting ENOENT or E125001 for @/@]] XFAIL: pegrev_parse_tests.py 83: rename 'iota' to 'E/@tau with pegrev escape [[Rename creates 'E/@tau@' instead of
Re: Pristines-on-demand: fix disabled tests (#4891)
On Thu, Apr 7, 2022 at 11:31 AM Julian Foad wrote: > Now can you run the test suite with --wc-format-version=1.15? On Unixy > systems that is done by, for example: > > $ make svnserveautocheck WC_FORMAT_VERSION=1.15 ... Okay, passing that option to win-tests.py (the test runner on Windows) failed. Should be fixed now, with r1899654. I suppose not passing the option uses wc-format-version=1.15 automatically? Or what is the default? Is format 1.8 the same as 1.14 and anything in between? Should I run with 'None' and 1.8? Or 1.8 and 1.15? -- Johan
Re: Pristines-on-demand: fix disabled tests (#4891)
On Thu, Apr 7, 2022 at 11:31 AM Julian Foad wrote: > Jun Omae wrote: > >> FAIL: diff_tests.py 48: svn diff --diff-cmd provides the correct arguments > > > > I've posted patch for the failure. > > See https://lists.apache.org/thread/2o0xtqfzy9xg8wzxscj2wb641p2kyo9c > > Thank you, Jun Omae. Sorry, I missed that before. Committed now in r1899645. Thanks, confirmed fixed on Windows. -- Johan
Re: Pristines-on-demand: fix disabled tests (#4891)
On Wed, Apr 6, 2022 at 5:12 PM Julian Foad wrote: > Johan Corveleyn wrote: > >> A few of these are Windows-specific. I can't very well investigate those > >> myself. Who could volunteer to look at those? They are: > >> > >> externals_tests.py ... ... ...: > >> update_modify_file_external(), > >> remap_file_external_with_prop_del(), > >> file_external_recorded_info(): > >> existing issue (Windows only) > >> > >> These tests are commented with "# Existing issue: `src_stream` not > >> closed in externals.c:apply_textdelta()" > >> > >> Does that mean it's a problem already on trunk? Or a hidden problem? If > >> someone could please take a look, that would be great. If not, could > >> anyone volunteer to test a possible fix if one of us unixers made a guess? > > > > I have no idea what those tests are about, but I can probably help > > test things on Windows. > > I'll try to get the branch built on my Windows machine tonight, and > > then I can help try out various things. > > Great! Thanks. > > I found also this one is a Windows-only test: > > update_tests.py 57 skip_access_denied(): access denied paths should > be skipped > > (In principle I don't see why we shouldn't test a similar access-denied > condition on unix-like systems, though the error code would be > different. Maybe the answer is the semantics would be so different it > would fail at a different place in the code path and so be a different > test case. I don't plan to pursue this.) > > I think the current state is this test will report XFAIL/WIMP with > pristines-on-demand enabled (currently meaning WC format 32) and XPASS > otherwise. > > This test was introduced in r1143071 with the log message "Make svn > update handle some access denied scenarios with a proper skip. This > makes it less likely that the database will be closed after updating with > working queue items left." > > I would guess that gracefully handling the case where the working file > on disk has this particular kind of OS lock isn't of primary concern. Just quickly reporting that I've got the branch built and up and running on Windows now. Running the test suite currently gives these (X)FAILS and XPASSES. Will look closer tonight ... [[[ XFAIL: diff-diff3-test 18: 3-way merge, double add XFAIL: dirent_uri-test 47: test match with RFC 6125 s. 6.4.3 Rule 3 XFAIL: op-depth-test 42: mixed_rev_move [[needs different libsvn_wc entry point]] XFAIL: op-depth-test 56: commit_moved_away_descendant XFAIL: op-depth-test 68: move retract (issue 4336) XFAIL: op-depth-test 69: move/delete file externals (issue 4293) XFAIL: op-depth-test 75: move more than once, revert intermediate XFAIL: op-depth-test 79: del4: delete AAA XFAIL: op-depth-test 80: del4: add AAA XFAIL: op-depth-test 81: del4: replace AAA XFAIL: op-depth-test 83: del4: replace self AAA XFAIL: op-depth-test 85: move4: delete AAA XFAIL: op-depth-test 86: move4: add AAA XFAIL: op-depth-test 87: move4: replace AAA XFAIL: op-depth-test 89: move4: replace self AAA XFAIL: op-depth-test 95: move within mixed move XFAIL: basic_tests.py 9: basic corruption detection on update [[Relies on wc.text_base_path()]] XFAIL: basic_tests.py 63: peg rev resolution on non-existent wc paths XFAIL: blame_tests.py 15: blame -g handles changes from empty mergeinfo XFAIL: changelist_tests.py 5: diff --changelist (wc-wc and repos-wc) XFAIL: commit_tests.py 66: last changed of copied subdir XFAIL: commit_tests.py 74: commit sees tree conflict on unversioned path XFAIL: copy_tests.py 105: copy and move conflicts XFAIL: depth_tests.py 49: deleted & moved items left untouched XFAIL: depth_tests.py 50: unversioned files in excluded directory XFAIL: diff_tests.py 77: diff repo to wc of a copy XFAIL: diff_tests.py 90: diff unversioned files in git format XFAIL: diff_tests.py 92: diff summary repo wc local copy unmodified XFAIL: diff_tests.py 94: diff git format copy XFAIL: export_tests.py 11: export working copy at base revision XFAIL: externals_tests.py 25: update that modifies a file external XFAIL: externals_tests.py 39: file external remap segfaults due to deleted props XFAIL: externals_tests.py 44: move with file externals XFAIL: externals_tests.py 49: file externals versioned obstruction XFAIL: externals_tests.py 68: check file external recorded info XFAIL: log_tests.py 46: log --use-merge-history --search XFAIL: log_tests.py 47: log --use-merge-history --xml XFAIL: merge_automatic_tests.py 16: cherry2_fwd XFAIL: merge_automatic_tests.py 17: cherry3_fwd XFAIL: merge_tests.py 49: avoid repeated merges for cyclic merging XFAIL: merge_tests.py 64: merge target with non inheritable mergeinfo XFAIL: merge_tests.py 114: don't inherit bogus mer
Re: Pristines-on-demand=enabled == format 32?
On Wed, Apr 6, 2022 at 4:28 PM Julian Foad wrote: > > Johan Corveleyn wrote: > > I think this was asked several times before, but I can't find the > > thread: is the pristines-on-demand behavior still unconditionally tied > > to format 32? Or is it that format 32 makes it _possible_ to enable > > pristines-on-demand? > > Currently it's tied to f32, but it's pretty clear it needs to be > uncoupled. The issue is: > > https://subversion.apache.org/issue/4889 "Pristines-on-demand: per-WC config" > > In principle we could address this later, only when a further new > version is released with a further new format and a further new feature > that users need to have available without enabling pristines-on-demand; > but it seems more responsible to uncouple it before we release it. > > I think that means #4889 is the other blocker issue. (Does anyone see > any way around it?) Ah, yes, I think that makes #4889 a blocker. I tried to suggest a slightly more flexible per-WC-config than just a yes/no flag, but rather an open-ended "pristine strategy" or "pristine storage strategy" value (of which we would now introduces two options: "full" and "on-demand" / "lazy" / whatever) [1] [2]. But maybe that's overdesign without having an idea about what other "pristine storage strategies" might require in additional config details. [1] https://lists.apache.org/thread/h7xomovdclcm91vrskvj8kb0dbm1jng5 [2] https://lists.apache.org/thread/n3j50zv4sssqjcjfnz44ht5ho9p6db3f -- Johan
Pristines-on-demand=enabled == format 32?
I think this was asked several times before, but I can't find the thread: is the pristines-on-demand behavior still unconditionally tied to format 32? Or is it that format 32 makes it _possible_ to enable pristines-on-demand? I would object to having pristines-on-demand=enabled coupled to simply having WC format 32. Enabling pristines-on-demand should be optional. Having the feature available as of format 32 sounds fine, but coupling it unconditionally to format 32 sounds wrong. What if 1.16 introduces format 33 with a nice new feature I want, but I want a pristine-full working copy with it? -- Johan
Re: Pristines-on-demand: fix disabled tests (#4891)
On Wed, Apr 6, 2022 at 4:02 PM Julian Foad wrote: > === Windows-specific issues > > A few of these are Windows-specific. I can't very well investigate those > myself. Who could volunteer to look at those? They are: > > externals_tests.py ... ... ...: > update_modify_file_external(), > remap_file_external_with_prop_del(), > file_external_recorded_info(): > existing issue (Windows only) > > These tests are commented with "# Existing issue: `src_stream` not > closed in externals.c:apply_textdelta()" > > Does that mean it's a problem already on trunk? Or a hidden problem? If > someone could please take a look, that would be great. If not, could > anyone volunteer to test a possible fix if one of us unixers made a guess? I have no idea what those tests are about, but I can probably help test things on Windows. I'll try to get the branch built on my Windows machine tonight, and then I can help try out various things. -- Johan
Re: Website prep work and questions (mainly about the 1.15 release notes)
On Wed, Apr 6, 2022 at 1:28 AM Nathan Hartman wrote: > On Tue, Apr 5, 2022 at 7:19 PM Mark Phippard wrote: >> On Tue, Apr 5, 2022 at 4:49 PM Johan Corveleyn wrote: >> > 4. Signature verified OK, but Mark's key not trusted, which, as Nathan >> > also said, is normal because it hasn't been crossed-signed by anyone >> > in my "web of trust". Okay, it's in the KEYS file (i.e. part of the >> > Apache records for Mark's id). This is as good as we can do, so +1. >> >> I am surprised that you all try to verify to this depth. I always just >> treated the signatures like a slightly better sha1 and did a simple >> gpg --verify to see if the signature was valid? Did you all cross sign >> each other's keys at one of the old developer meetups or something? > > gpg --verify checks if the key is in your web of trust automatically and > prints a warning if not, so it wasn't anything special we had to do. > > Perhaps other SVN devs crossed signed each other's keys in past hackathons > but mine isn't cross signed yet. Yes, at various occasions in the past we did have so called "keysigning parties", where we exchanged / cross-signed each others keys. At all of the SVN Hackathons hosted by Elego in Berlin for instance. I suppose other Apache events do them too. I don't remember the exact details, but I found some background here: https://infra.apache.org/release-signing.html#web-of-trust https://infra.apache.org/release-signing.html#key-signing-party -- Johan
Re: Website prep work and questions (mainly about the 1.15 release notes)
Thanks all for sharing your gpg key hurdles. It saved me a lot of time when I ran into the same issues while verifying Mark's signature :-). 1. Signature algorithm not recognized -> updated my gpg to latest version (2.3.4) 2. keyserver problem when running 'gpg --refresh-keys' -> put 'keyserver hkp://keyserver.ubuntu.com' into my %APPDATA%/gnupg/gpg.conf like Julian did 3. Mark's key unknown -> executed 'gpg --recv-key EC25FCC105618D04ADB43429C4416167349A3BCB' to get it 4. Signature verified OK, but Mark's key not trusted, which, as Nathan also said, is normal because it hasn't been crossed-signed by anyone in my "web of trust". Okay, it's in the KEYS file (i.e. part of the Apache records for Mark's id). This is as good as we can do, so +1. Phew -- Johan
Re: Subversion 1.10.8 up for testing.signing
On Sat, Apr 2, 2022 at 3:27 PM Mark Phippard wrote: > > The 1.10.8 release artifacts are now available for testing/signing. > > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. > > Thanks! Summary --- +1 to release Platform Windows 10 x64 (Version 1903) Microsoft Visual Studio 2019 Community Edition (Version 16.5.3) Verified Signature, sha1 and sha512 for subversion-1.10.8.zip. Contents of subversion-1.10.8.zip are identical to tags/1.10.8, and to branches/1.10.x@1899510 (except for expected differences in svn_version.h and svnpubsub, svnwcsub and nominate.pl (symlinks vs. file contents), and generated files). Tested -- [ Release build x64 ] x [ fsfs ] x [ file | svn | http ] Results --- All tests pass. Dependencies httpd 2.4.43 (apr 1.6.5, apr-util 1.6.1, pcre 8.44, expat 2.2.9, openssl 1.1.1g) apr 1.6.5 apr-util 1.6.1 openssl 1.1.1g serf 1.3.9 sqlite 3.31.1.0 zlib 1.2.11 (bundled lz4 1.7.5) (bundled utf8proc 2.1.0) Other tools --- perl 5.30.2 (Strawberry Perl) python 2.7.17 Signature - subversion-1.10.8.zip: -BEGIN PGP SIGNATURE- iQIzBAABCgAdFiEEiqLBDuqtRPlpcnrqtZzm1gEMiq0FAmJMpwUACgkQtZzm1gEM iq0gTxAAoa4wBU3ZEVm0b5BLxQpJN8pctERN2u8ayEqDFM8Zbm+5DAq+LDbAJQ44 1OJia3yEYpBNGWu8v3qoC7XNQ/f7QXHhMNnUMCm82bwjHufnib1qzUJZYLHSK9gy iQqTRU81Jo+C8aPrbRjzqEX6j6sckv23+0vHYqZYYBKLZ8BOOwcXEzSfmXrk9shR m/794W2OweXaJ4bfD1dYUH3viUuRoCTygQYEErmpIC4c8I/CMDXYeCp93mMJoZwf 8+ReJJ1Zsq1ffnGtv+TDcRY0xMFveKdtOrbvFn16Eu3mUgIyD2VQEGjj4e/1alyv DgRibiHlT6YIwOgSHf+G6RJp3NH78jVf0t3UY8bdrAe5klExbQ8BkW5xs/833bJW KMCKpfnWakvnn4sYw0P1wbUm+gRukhQ0vklegyOD2kO4RGGlMHXvaSA6qjONvmaa n7wRJUTK4xEL+wLe0sXQ0ARmZwLNJTbSQjdsjH5v/n40jDzXUCrynI5XEox5nT74 hSBq2CKjm0OY9jVMol6GgzYzD6kxR9/IHHwMJaNolLw99CJqCKVsYC2dNbSCmP5L qmwvprB2ofXxG4R1eJ7urebfIOx6B0hmev8aw7KVQpcrm2Wp2WS+FG+jKn0tHMTW SJkrvo+HS8oS5sX5uc2e6ieGjYIaoobANq4XOBCtE09oNDCAUv8= =Vigs -END PGP SIGNATURE- -- Johan
Re: Subversion 1.14.2 up for testing/signing
On Sat, Apr 2, 2022 at 3:28 PM Mark Phippard wrote: > > The 1.14.2 release artifacts are now available for testing/signing. > > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. > > Thanks! Summary --- +1 to release (Windows) Platform Windows 10 x64 (Version 1903) Microsoft Visual Studio 2019 Community Edition (Version 16.5.3) Verified Signature and sha512 for subversion-1.14.2.zip. Contents of subversion-1.14.2.zip are identical to tags/1.14.2, and to branches/1.14.x@1899510 (except for expected differences in svn_version.h and svnpubsub, svnwcsub and nominate.pl (symlinks vs. file contents), and generated files). Tested -- [ Release build x64 ] x [ fsfs ] x [ file | svn | http ] javahl swig-python Results --- All tests pass. Dependencies httpd 2.4.43 (apr 1.6.5, apr-util 1.6.1, pcre 8.44, expat 2.2.9, openssl 1.1.1g) apr 1.6.5 apr-util 1.6.1 openssl 1.1.1g serf 1.3.9 sqlite 3.31.1.0 zlib 1.2.11 python 3.9.1 py3c 1.0 swig 4.0.1 (bundled lz4 1.7.5) (bundled utf8proc 2.1.0) Other tools --- perl 5.30.2 (Strawberry Perl) python 3.9.1 AdoptOpenJDK 11.0.6 junit 4.12 Signature - subversion-1.14.2.zip: -BEGIN PGP SIGNATURE- iQIzBAABCgAdFiEEiqLBDuqtRPlpcnrqtZzm1gEMiq0FAmJMpmQACgkQtZzm1gEM iq30HA/8CCJwOHHYmx6qzumeTxmiW+ZORSGYRn52j3C2EQOZPfbeKXkNQh052ntx 3JI3GMhlQPhms/QzDyDqfMy6zw6quVhP1BKNR4fjcts3SC+bjUcbplwEUM38BiA8 RZiaAlO7fwIjgEFLSixKKwL/+RAr+U35Q8yPw2gtszMTIOZVg8MBKWmNC1fv5GJN 64lDGCtnzgPdZTcNsvaVqXCpPl1O8hNmLCPSQyLzom5UsNy6wkW1VEHJqA8dXm4K tMpuGURW/8NBjKlXOT/gg7TijjXKHk8PqyFcCEUFzpAdL/V2/wzO4W3FT1yD1UiL y1837e2PUWDydeAnt2atYxjm3BiGiBiJEdfy1MIKm508KNRQy5E+0VOaN4F9B0pe ZrmPh9t2IzxdB+AWbYS74BFQUHXc93LNjQ4abQl7zWZkcNibkNN32lULNgLqurEG RKSBgRyycxL6uzyovuhiLGI7WA+uOC1Bu1CD5KYrhrSWtoBFEf8pV42w+Loi6SHm xCrgbycWZbDzmrFzlsnTpHvgq60aKJodaIAox4pJkd5Xx1vTSkLs8bdGs8xy0j8a mc7kM1NJ8p+8wpPvjqS7rBKfEy3uPbgNyPrsExcbowNZk0L/QC4XYKvNanQRvsC/ 92AoiwxxsoOD24r4eRCN2x53TfE303X9f9mlQg4JKwwGnlI1PPA= =v/9k -END PGP SIGNATURE- -- Johan
Re: [PATCH] limit diff effort to fix performance issue
On Fri, Apr 1, 2022 at 5:19 PM Stefan Sperling wrote:
>
> On Fri, Apr 01, 2022 at 05:04:49PM +0200, Johan Corveleyn wrote:
> > Yes, I suppose this is the case: Patience feeds different (smaller)
> > things to LCS. Because, as far as I understand, Myers' way of
> > calculating the LCS is fundamentally "somewhat" quadratic (according
> > to the Myers paper from 1986 [1], titled "An O(ND) Difference
> > Algorithm and Its Variations").
> >
> > That is: O(ND) where N = the sum of the lengths of both sides of the
> > diff and D = the size of the minimal diff. That's why eliminating the
> > common prefix and suffix (added as an optimization years ago) is
> > useful, it makes N much smaller.
> >
> > Now, in cases where both texts are huge (even after prefix-suffix
> > stripping), but the minimal diff is small, the algorithm should in
> > theory be fast (because D is small). Just not sure what is going wrong
> > then in our variation of this algorithm. Or have we implemented
> > another algorithm than the one described by Myers?
>
> SVN diff is allegedly "based on the approach described by ... Meyers (sic),
> [...] but has been modified for better performance."
> I guess the modifications refer to prefix/suffix scanning,
> because this description dates from r1128862.
Hm, not the prefix/suffix scanning, as I implemented those (it was my
first big patch / work in SVN), and that's outside of the LCS
algorithm (it happens in a phase before assembling "tokens (=lines)"
and before handing it off to the LCS algorithm). It was integrated
into trunk in r1067800 (and some further fixes followed afterwards).
Basically, that work is done in
libsvn_diff/diff_file.c#find_identical_{prefix,suffix}.
I suppose the "modified for better performance" refers to some
optimisations done by Morten Kloster, who then later submitted the
patch adding this comment in r1128862. His optimisations were more
related to the LCS algorithm (further reducing the problem space, and
providing early exits in certain cases or some such).
The core LCS algorithm in libsvn_diff/lcs.c was from way before my
time, and according to 'svn blame' was mainly written by Sander
Striker. I don't understand it fully either :-), but I always assumed
it was basically some clever implementation of Myers' algorithm.
--
Johan
Re: [PATCH] limit diff effort to fix performance issue
On Fri, Apr 1, 2022 at 4:37 PM Stefan Sperling wrote: > Yes, roughly, Patience diff involves two algorithms, the grouping of > lines along similar-line boundaries performed by Patience, and an > LCS for parts of the files which Patience cannot deal with by itself. > > But LCS needs to complete its work before Patience can do its thing, > and vice-versa. For a given input, each algorithm might run multiple > times, switching whenever the current algorithm cannot make any process. > So if LCS still has a fundamental performance issue for some inputs, > adding Patience diff probably won't help. It could help in cases where > LCS is now fed with different inputs as determined by Patience, such > that bad inputs for LCS are avoided. But I don't know if that would > always be the case. Yes, I suppose this is the case: Patience feeds different (smaller) things to LCS. Because, as far as I understand, Myers' way of calculating the LCS is fundamentally "somewhat" quadratic (according to the Myers paper from 1986 [1], titled "An O(ND) Difference Algorithm and Its Variations"). That is: O(ND) where N = the sum of the lengths of both sides of the diff and D = the size of the minimal diff. That's why eliminating the common prefix and suffix (added as an optimization years ago) is useful, it makes N much smaller. Now, in cases where both texts are huge (even after prefix-suffix stripping), but the minimal diff is small, the algorithm should in theory be fast (because D is small). Just not sure what is going wrong then in our variation of this algorithm. Or have we implemented another algorithm than the one described by Myers? Anyway, if Patience diff is able to split up the work for LCS in smaller bits (with their small O(ND) complexity), then I can totally understand that this can be faster. [1] http://www.xmailserver.org/diff2.pdf -- Johan
Re: [PATCH] limit diff effort to fix performance issue
On Fri, Apr 1, 2022 at 1:12 PM Stefan Sperling wrote: > > On Fri, Apr 01, 2022 at 12:44:24PM +0200, Johan Corveleyn wrote: > > On Tue, Jun 8, 2021 at 5:58 PM Johan Corveleyn wrote: > > > On Tue, Jun 8, 2021 at 3:24 PM Stefan Sperling wrote: > > > > On Tue, Jun 08, 2021 at 02:57:34PM +0200, Johan Corveleyn wrote: > > > > > Okay, I focused on the first revision causing the annotate to differ, > > > > > and with some debug logging: > > > > > - p went up to 139 > > > > > - length[0]=1942, length[1]=1817 > > > > > > > > > > Now, 1942 lines on the left and 1817 on the right doesn't seem all > > > > > that many (that's what remains after prefix-suffix stripping). I see > > > > > no reason 'svn diff' shouldn't be able to calculate a minimal diff for > > > > > those sizes in a reasonable amount of time. So if p can go up to such > > > > > a "cost" for such "reasonable" lenghts, I imagine we should put the > > > > > actual limit much higher. I suppose we can just as well set "min_cost" > > > > > to 1024 or even higher. 64 is way too low. > > > > > > > > Thanks! > > > > I have set the value back to at least 1024 with this new version of > > > > patch. > > > > > > Hmmm, apparently I'm still running into the limit. Even with 8192 I'm > > > hitting it at least once. The actual diff there is not really pretty, > > > but it's an actual manual change made by some developer years ago, and > > > the "minimal diff" is more or less readable / understandable. The log > > > message is something like "Group sections related to multimedia > > > module", and it shuffles around a lot of xml sections to group them > > > together. > > > > > > In this case, length[0]==length[1]==46780. Pretty big for the LCS, but > > > not humongous. The value of 'p' goes up to 8279. > > > > > > With the limit set to 16384, I'm not hitting it, and the annotate > > > comes out as with the original. > > > > > > Now, I'm a bit puzzled why limit==8192 already takes 18s in your > > > tests. In my situation, with the above diff, I get: > > > limit==8192: 3.3s > > > limit==16384: 3.5s > > > > > > (that's including downloading both versions from the server over https) > > > > > > So I'm wondering why it takes so long in your case. One thing to keep > > > in mind here is that, since this is cpu intensive, optimizations might > > > matter. I remember back in 2011 that I did most of my measurements > > > with a "Release" build for example. But the above tests I did were > > > with a debug build, so ... dunno. > > > > [ snip part about another optimization by Morten Kloster in r1140247, > > which seemed not to work for Stefan. ] > > > > Coming back to this thread from last year, just wondering if you want > > to pick this up again, Stefan (or anyone else), since a 1.15 release > > might be happening in the near future. > > > > I think we already concluded that changing this behaviour in a patch > > release would be hard to justify. But for 1.15 it might be acceptable > > in some form. > > > > Just wanted to bring this back up, I have no vested interest here. I'm > > still skeptical about changing the output of diff & blame in cases > > where we hit "this takes too much work", but I won't stand in the way > > if that what others consense on :-). If we want to pick a limit of the > > maximum effort, then I'd suggest 16384 (or above), but that's a purely > > personal / local suggestion, because the use case I had above would > > then not be impacted. > > > > BTW: it's a pity that we can's say: "limit the diff effort in case of > > 'update' or 'merge', but not in case of 'diff' or 'blame'". Because in > > the latter case, the user might care more about the actual output, and > > in the former they might not (until they hit a text conflict of > > course, then they will care again ...) > > > > It might be interesting if someone could take a more "profiling" look > > at why Stefan's example takes much more time, even with limit==8192 > > (18 seconds), compared to my example (3.3 seconds). Is that purely > > because of the total size of the files / number of lines? I find that > > difference quite strange. There might be something that can be > > optimized without changing behaviour. But I'
Re: [PATCH] limit diff effort to fix performance issue
On Tue, Jun 8, 2021 at 5:58 PM Johan Corveleyn wrote: > On Tue, Jun 8, 2021 at 3:24 PM Stefan Sperling wrote: > > On Tue, Jun 08, 2021 at 02:57:34PM +0200, Johan Corveleyn wrote: > > > Okay, I focused on the first revision causing the annotate to differ, > > > and with some debug logging: > > > - p went up to 139 > > > - length[0]=1942, length[1]=1817 > > > > > > Now, 1942 lines on the left and 1817 on the right doesn't seem all > > > that many (that's what remains after prefix-suffix stripping). I see > > > no reason 'svn diff' shouldn't be able to calculate a minimal diff for > > > those sizes in a reasonable amount of time. So if p can go up to such > > > a "cost" for such "reasonable" lenghts, I imagine we should put the > > > actual limit much higher. I suppose we can just as well set "min_cost" > > > to 1024 or even higher. 64 is way too low. > > > > Thanks! > > I have set the value back to at least 1024 with this new version of patch. > > Hmmm, apparently I'm still running into the limit. Even with 8192 I'm > hitting it at least once. The actual diff there is not really pretty, > but it's an actual manual change made by some developer years ago, and > the "minimal diff" is more or less readable / understandable. The log > message is something like "Group sections related to multimedia > module", and it shuffles around a lot of xml sections to group them > together. > > In this case, length[0]==length[1]==46780. Pretty big for the LCS, but > not humongous. The value of 'p' goes up to 8279. > > With the limit set to 16384, I'm not hitting it, and the annotate > comes out as with the original. > > Now, I'm a bit puzzled why limit==8192 already takes 18s in your > tests. In my situation, with the above diff, I get: > limit==8192: 3.3s > limit==16384: 3.5s > > (that's including downloading both versions from the server over https) > > So I'm wondering why it takes so long in your case. One thing to keep > in mind here is that, since this is cpu intensive, optimizations might > matter. I remember back in 2011 that I did most of my measurements > with a "Release" build for example. But the above tests I did were > with a debug build, so ... dunno. [ snip part about another optimization by Morten Kloster in r1140247, which seemed not to work for Stefan. ] Coming back to this thread from last year, just wondering if you want to pick this up again, Stefan (or anyone else), since a 1.15 release might be happening in the near future. I think we already concluded that changing this behaviour in a patch release would be hard to justify. But for 1.15 it might be acceptable in some form. Just wanted to bring this back up, I have no vested interest here. I'm still skeptical about changing the output of diff & blame in cases where we hit "this takes too much work", but I won't stand in the way if that what others consense on :-). If we want to pick a limit of the maximum effort, then I'd suggest 16384 (or above), but that's a purely personal / local suggestion, because the use case I had above would then not be impacted. BTW: it's a pity that we can's say: "limit the diff effort in case of 'update' or 'merge', but not in case of 'diff' or 'blame'". Because in the latter case, the user might care more about the actual output, and in the former they might not (until they hit a text conflict of course, then they will care again ...) It might be interesting if someone could take a more "profiling" look at why Stefan's example takes much more time, even with limit==8192 (18 seconds), compared to my example (3.3 seconds). Is that purely because of the total size of the files / number of lines? I find that difference quite strange. There might be something that can be optimized without changing behaviour. But I'm afraid I don't have time to dig deeply into this. -- Johan
Re: r1897441 (ping to jun66j5 and futatuki)
On Thu, Mar 31, 2022 at 1:25 AM Jun Omae wrote: > > On Thu, Mar 31, 2022 at 3:00 AM Johan Corveleyn wrote: > > Mark nominated it, and I just tested it again. However, r1884474 only > > seems to fix mod_authz_svn_tests.py#10. > > > > mod_authz_svn_tests.py#11 still fails for me after merging r1884474. I > > haven't looked into it, and have to run right now. It's very well > > possible that I have done something wrong or that my test-setup is > > broken locally, don't know yet. > > > > In case anyone want to take a closer look, here is the dav-fails.log > > in attachment. If no one else beats me to it, I'll take another look > > later tonight or tomorrow. > > In my environment (Windows 10), 2 failures go away after merging r1884474: > > [[[ > ['httpd.exe', '-f', > 'C:\\usr\\src\\subversion\\1.14.x-py310\\Release\\subversion\\tests\\cmdline\\httpd\\httpd.conf'] > Testing Release configuration on remote repository http://localhost:10229. > [1/1] > mod_authz_svn_tests.pysuccess > At least one test XFAILED, checking > C:\usr\src\subversion\1.14.x-py310\Release\dav-tests.log > XFAIL: mod_authz_svn_tests.py 3: test mixed with noauthwhenanon directive > Summary of test results: > 10 tests PASSED > 1 test XFAILED > Python version: 3.10.0. > SUMMARY: All tests successful > > > The connection was closed by httpd.exe in mod_authz_svn_tests.py#11, > but no hints for the reason. > [[[ > ConnectionResetError: [WinError 10054] De externe host heeft een > verbinding verbroken > ]]] > Please check Release\subversion\tests\cmdline\httpd\log file in your > environment. > Ah, thanks. I've sorted it out. Seems to work now. I'll vote for the backport shortly. Thanks for you help, Jun! -- Johan
Re: r1897441 (ping to jun66j5 and futatuki)
On Wed, Mar 30, 2022 at 8:28 AM Daniel Sahlberg wrote: > > Den ons 30 mars 2022 kl 00:54 skrev Jun Omae : >> >> On Wed, Mar 30, 2022 at 6:20 AM Daniel Sahlberg >> wrote: >> > >> > Hi, >> > >> > Does anyone with experience in the bindings have time to look at the >> > backport of r1897441 (in 1.14.x/STATUS)? It was nominated by jun66j5, >> > however it doesn't apply cleanly. I think it would work if at least >> > r1885008 is added to the group, but probably also r1886858 and r1886872. >> > >> > It would be good to have it merged if it increases test coverage. >> > >> > Kind regards, >> > Daniel >> >> Sorry, I noticed that the nomination r1897441 is not needed for 1.14.x. >> Removed it from STATUS file. >> >> However, I get 2 failures from 1.14.x/win-tests.py on Windows >> (tests for swig-py, swig-pl and javahl pass). >> >> FAIL: mod_authz_svn_tests.py 10: repos-relative access file >> FAIL: mod_authz_svn_tests.py 11: repos-relative access file with bad >> repository URL >> >> It seems to be necessary to merge r1884474 to fix. > > > That one seems quite straight forward. Feel free to nominate! Mark nominated it, and I just tested it again. However, r1884474 only seems to fix mod_authz_svn_tests.py#10. mod_authz_svn_tests.py#11 still fails for me after merging r1884474. I haven't looked into it, and have to run right now. It's very well possible that I have done something wrong or that my test-setup is broken locally, don't know yet. In case anyone want to take a closer look, here is the dav-fails.log in attachment. If no one else beats me to it, I'll take another look later tonight or tomorrow. -- Johan dav-fails.log Description: Binary data
Generating CHANGES
In the thread with subject "Re: svn commit: r1899324 - /subversion/site/publish/upcoming.part.html": On Mon, Mar 28, 2022 at 10:14 PM Daniel Sahlberg wrote: > > Den mån 28 mars 2022 kl 22:04 skrev Mark Phippard : >> >> What is this file used for? Is it displayed on the website somewhere? > > > Yes, it is included in the release-notes page (via an SSI #include): > https://subversion.apache.org/docs/release-notes/ > >> It is just showing merged changes since the last release? So I guess >> could help us write CHANGES? > > > Yes, it should only show merged changes since last release. It didn't work > properly and showed changes since 1.14.0 but I hope it should be alright now. > It is generated by site/tools/generate-upcoming-changes-log.sh (you should be > able to run it locally, but make sure that `cwd` is in the 1.14.x branch). > And yes, it would probably be a good starting point for CHANGES. For generating CHANGES you can also try the 'write-changelog' subcommand of tools/dist/release.py, with the --include-unlabeled-summaries option, and '--branch branches/1.14.x': [[[ $ release.py write-changelog -h usage: release.py write-changelog [-h] [--include-unlabeled-summaries] previous positional arguments: previous The "previous" branch or tag, relative to ^/subversion/, to compare "branch" against. options: -h, --helpshow this help message and exit --include-unlabeled-summaries Include summary lines that do not have a changes label, unless an explicit [c:skip] or [c:ignore] is part of the commit message (except if the summary line contains 'STATUS', 'CHANGES', 'Post-release housekeeping', 'Follow-up' or starts with '*'). ]]] For example: [[[ release.py --branch branches/1.14.x write-changelog --include-unlabeled-summaries tags/1.14.1 * -1 vote for r1892471 as explained in dev@. (r1892509) * Add test coverage for CVE-2020-17525 (mod_authz_svn NULL deref) (r1899256) * Document how the port number is passed to custom tunnels. (r1899258) * Fix a bug where «make davautocheck» immediately after configure failed hard because of an unbuilt dependency. (r1899242) * Fix an error message when running make davautocheck. (r1899340) * Fix encoding of error message on failure of system() call. (r1899257) * Fix issue #4880, "Use-after-free of object-pools when running in httpd" (r1899339) * Fix misleading -r option documentation for some svnadmin subcommands. (r1896935) * Follow up to r1865987, r1866588: Unbreak a msgid. (r1899241) * Reverting my recent commit r1892122 (backporting r1892121). It depends on (r1892123) * Upgrade my vote for the r1892470 group. (r1892547) * Use the APR-1.4+ API for flushing file contents to disk. (r1899341) * swig-py: Fix dependency of make copy-swig-py target. (r1899255) * swig-py: Fix doubly destroying memory pool with cyclic garbage collector. (r1889654) * swig-py: Skip some tests on Python 2 if default encoding is 'utf-8'. (r1899243) * tests: Include additional information in an error message. (r1899259) User-visible changes: Developer-visible changes: ]]] This is just a skeleton of course. If nothing else, you can take a look at it to see if you missed something. It extracts either lines from STATUS, or "summary lines" (first lines of commit messages) of revisions that are in branches/1.14.x but not in tags/1.14.1 (it uses 'svn mergeinfo --show-revs eligible ^/branches/1.14.x ^/tags/1.14.1' for determining those). Auto-generating the changelog would get more powerful if we'd adopt some commit message conventions for "tagging" / "labeling" commit messages immediately when they are committed for the first time, but this idea never got much traction on dev@ [1], so now this is pretty basic (just extracting "summary lines" with revision numbers and putting them in a list). [1] https://svn.haxx.se/dev/archive-2017-12/0020.shtml -- Johan
Re: Issue #525/#4892: on only fetching the pristines we really need
On Wed, Mar 16, 2022 at 9:59 PM Julian Foad wrote: > Daniel Shahaf wrote: > > Also, why is this specific to «svn update»? > > It's not specific to update. Update is a particular case that Karl cares > about so I looked at doing "update" first. Implementing this approach in one > subcommand at a time could be considered releasable incremental steps, > because each one is a further optimisation. It's not specific to 'svn update' per se, but it's logical that it leads to this discussion, because it is a (commonly used) case where the pristine is not actually needed for the operation (if there is no actual incoming update to the concerned file). 'svn diff' and 'svn revert' cannot do their work without the pristine, but 'update without an actual incoming edit'? And even with an 'incoming edit on update (on top of local mod)' it might in theory be possible to delay the download of the full pristine until after conflict-resolution decision (but I imagine that's even more difficult to untangle). Also, why should 'svn update' be in the business of (silently) restoring "the branch's invariant" (even when it does not need the file), and not any other operation (like 'svn status -u' for example)? -- Johan
Re: Issue #525/#4892: on only fetching the pristines we really need
On Mon, Mar 14, 2022 at 5:26 PM Julian Foad wrote: > > Johan Corveleyn wrote: > >Speaking from the peanut gallery, [...] > >If I would be a user with several huge binaries in the repo / WC, I > >imagine I would not be happy with this proposal. The reason is that I > >have always, forever, only done "svn update the-whole-wc". Updating > >individual subdirs is micro-managing. [...] > > But do you locally modify those huge binaries? Well, as I said, I don't have huge binaries myself (nor do my colleagues, for that matter). We have a single build server with 400 working copies, each with 10's of "normal sized" files. But apparently some users, like Karl (or Karl's customer), do have huge binaries in their WC, and I suppose sometimes they will be modified. All I can say is: individual-directory-updating is not something I've often witnessed being used (unless maybe for a short ad-hoc purpose, not on a regular basis). With lots of things going on, and lots of things being worked on at the same time, the usual workflow I know is "update entire WC to keep up with others; start working (or continue where you left yesterday)". So if I would locally modify a huge binary (it might be left there for days, weeks, ... locally modified, until I'm happy for it to be committed), every time I update I would update the entire WC. That's just me speculating of course ... maybe I should leave the floor for Karl and others ... Just one more thing: if people "micro-manage" their working copy, in my experience they will usually do this once, by setting up an appropriately arranged sparse working copy (leaving out for example the huge binaries of other teams they're not interested in, or source modules that are not important for their work). After this one-time setup (now and then finetuned to exclude or add another directory or file), people just "svn update entire-sparse-wc". It makes no sense to me to update only subdirectories, at least not on a regular basis. That's too much fiddly work everytime (and you can't save that "to-be-updated selection" for next time), and you might end up with inconsistent stuff locally (for instance a library not being updated together with its callers). But again, I might have wrong assumptions here, since I don't work with huge binaries in SVN myself. -- Johan
Re: Issue #525/#4892: on only fetching the pristines we really need
On Mon, Mar 14, 2022 at 11:48 AM Julian Foad wrote: > > Dear dev community, and especially Karl and Mark: > > A plea to test the current design/implementation. > > I wonder if we are missing some perspective. > > We are worried that the current design won't be acceptable because it > has poor behaviour in a particular use case. > > The use case involved running "svn update" at the root of the WC. (It > didn't explicitly say that. More precisely, it implied the update target > tree contains the huge locally modified file.) > > Using this new feature necessarily requires some adjustments to user > expectations and work flow. > > What if we ask the user to limit their "svn update" to target the > particular files/paths that they need to update, keeping their huge > locally modified file out of its scope? Examples: > > svn update readme.txt > svn update small-docs/ > # BUT NOT: svn update the-whole-wc/ > > Then we side-step the issue. It only fetches pristines for modified > files that are within the tree scope of the specified targets. (This is > how it works already, not a proposal.) > > OK that's not optimal but it might be sufficient. Speaking from the peanut gallery, as I or "my users" probably won't be bothered too much by the occasional "superfluous pristine" (as I said before, I'm more interested in this feature for our build server with 400 working copies -- not particularly for rare huge files, but more for the general overhead of duplicating thousands of files): If I would be a user with several huge binaries in the repo / WC, I imagine I would not be happy with this proposal. The reason is that I have always, forever, only done "svn update the-whole-wc". Updating individual subdirs is micro-managing. I update my entire project (i.e. working copy) daily just to keep up with whatever my 100 colleagues have done since the last time I updated. It's just daily routine: Press Ctrl-T (Update Project) in IntelliJ, and start working. I'm not going to bother thinking about what exactly I need updated, and multi-selecting the 80 subdirs that I might need to keep in sync with the most important stuff. That's just my opinion, or my feeling trying to place myself in the position of such users. I'm not doing any actual work here, so I certainly don't want to stand in the way of consensus / progress, if the current implementation is sufficient for others. -- Johan
Re: Issue #525/#4892: on only fetching the pristines we really need
On Fri, Mar 11, 2022 at 9:17 PM Nathan Hartman wrote: > If possible and not overly burdensome, I think it would be a good > thing to keep the "restore" functionality for the following reasons: [snip] I agree. I know about the restore feature too, and am used to it. Also, I think it would be a mistake to create different behaviour between "normal (pristine-full)" and "pristines-on-demand" working copies. On "update's essential functionality": the main problem, I feel, is that the current implementation fetches missing pristines for modified files *even if they will not be updated*. I understand that this happens because, at the point of "textbase-sync" (check for and download missing pristines, which happens before the operation goes into details), it doesn't know yet which files will effectively receive an update. So this really feels like a waste (fetching stuff which will not even be needed, and indeed in many use cases like Karl's often won't be). But you all know that already, I guess, and I appreciate all efforts in trying to come up with creative approaches to avoid this "waste" :-). Don't know if this is realistic or useful, but: Apart from pushing this textbase-sync closer to the point of access, one could imagine that during textbase-sync the client would already have more information about what will be updated later on. For example by organizing the client-server exchange so that the client first receives a report, without texts, about what will be updated, then fetching missing pristines if needed, and then going on to request the actual update texts. IIRC, this kind of exchange is already the default since 1.8 with serf (except if certain directives / config flags are set) [1]. Then again, it might be quite awkward to implement this kind of flow only for one particular variation of our client-server protocol. [1] https://subversion.apache.org/docs/release-notes/1.8.html#serf-skelta-default -- Johan
Re: A two-part vision for Subversion and large binary objects.
On Mon, Feb 14, 2022 at 11:13 AM Ivan Zhakov wrote:
> On Mon, 14 Feb 2022 at 01:39, Karl Fogel wrote:
>> On 12 Feb 2022, Mark Phippard wrote:
...
>> In any case, the branch name doesn't matter too much here,
>> especially since it's going to get merged soon. However, for the
>> user-facing name of the feature, we should pick a name based on
>> the essence of the feature, not on a not-yet-fully-implemented
>> optional enhancement to the feature, discussed further below.
>>
>> On 13 Feb 2022, Julian Foad wrote:
>> >That name came, as far as I am aware, from Evgeny's branch which
>> >implements the latter.
>> >
>> >This may be a case where the public facing name for the feature
>> >ought to differ from the internal development name.
>> >
>> >Any ideas for a good public name?
>> >
>> >Pristines on Subversion's demand?
>> >Dehydrated WC?
>>
>> I kind of like the dehydration/rehydration theme -- it's certainly
>> memorable! Other possibilities:
>>
>> - blob-optimized checkouts
>>
>> - "blobtimized" checkouts (okay, kidding there... :-) )
>>
> I would suggest:
> - optional pristines
As I tried to explain before, I think it makes more sense (also to new
users who have never used pre-1.15) to try to expose the feature as a
knob for the pristine storing (or caching) strategy. Because,
effectively, the pristine store is just a cache, right? All the
information is there on the server, and the client simply duplicates /
caches that information locally to make some operations more
efficient. Up until know, the pristine caching strategy was fixed:
"cache them all, all the time, forever".
So now we're working on a very lazy or minimal type of pristine
caching strategy (or "no caching", if you will -- we might consider it
an implementation detail that a pristine is fetched in the "regular
pristine store" for a moment, and cleaned up after the operation -- it
might just as well have been spooled to a tmp location, or in memory,
or ... during the operation).
To expose this to users, I would take a step back, and open the door
for other types of pristine caching strategy in the future. So I'd
say:
"New feature in 1.15: Configurable Pristine Caching", or "Flexible
Pristine Caching" or "Pristine Caching Options". Where it was
previously a fixed strategy, you now have some choice. In 1.15 we
introduce the "lazy" (or "short-lived", or "minimal") pristine caching
strategy. Apart from that we still have the (default, old) "full" /
"complete" caching strategy. In the future we might introduce
additional (more flexible) strategies, such as those dictated by some
rules, potentially with a repos-side suggestion (like with
svn:auto-props).
Instead of taking about "Pristine Caching Strategy", we could also
talk about the "Pristine Strorage Strategy" or "Storing Strategy"
('storing' instead of 'fetching', as the former is the more permanent
effect; fetching might be seen as an implementation detail on what
subversion needs to do when it runs into a non-stored pristine).
--
Johan
Re: Streamlining Subversion patch releases
On Wed, Feb 9, 2022 at 2:19 PM Stefan Sperling wrote: > > On Wed, Feb 09, 2022 at 08:01:26AM -0500, Mark Phippard wrote: > > Anyway, my feeling has been that one of the blockers to being RM is > > motivation. My feeling has been that it is a fair amount of work that > > might not go anywhere because we do not have enough interest in > > reviewing and signing the release. So why put in the effort to do this > > when the votes are not going to happen? > > It is a bit of a chicken and egg problem. We have never had a problem > getting the release out the door once an RM did step up. Enough people > always joined in to help once the ball got rolling. Indeed. In recent years we never had a problem getting enough PMC testers and signatures. In fact, getting my build environment dusted off again, and testing and signing releases, is about the only thing I still do around here :-). So, dear RM, just give me a tarball (or release zip I should say for Windows, I guess), and I'll give it a go. > > We have also always had what I > > thought was a peculiar policy that the RM's votes did not "count". So > > often the most motivated member of our community steps forward to do > > the RM work, and now we have lost the one sure thing vote we would > > have had for the release. > > We have already made the RM's vote count during our most recent releases, > as far as I remember. At least I did it that way to expedite the process. > The ASF requires release signatures by at least 3 PMC members. Any further > restrictions are self-imposed and can be avoided if needed. Yes, the last couple of releases we relaxed our old "3 *nix + 3 windows" requirement to just "3 PMC sigs (of which at least 1 *nix and 1 Windows)" [1]. That last part (1 *nix and 1 Windows) is a Subversion-PMC-self-imposed requirement, which could in theory be lifted (depending on consensus of the PMC), but I'm not sure we should. In any case, so far we've never been blocked by this. For me that's also one of the main differences between votes in STATUS and votes on the release. The voting requirements are different (not only the "1 *nix and 1 Windows", which applies only to release votes; but also the rule that backports of bindings and a couple of other things only require 2 votes in STATUS). Also: a vote in STATUS does not necessarily imply that I have run the entire testsuite across 3 ra flavours etc. So a vote there does not mean the same thing as a release vote. [1] https://subversion.apache.org/docs/community-guide/releasing.html#releasing-votes -- Johan
Re: A two-part vision for Subversion and large binary objects.
Replying to a few different points in this thread. On Jan 27, Julian Foad wrote: > The user can choose one mode, per WC, from a list of options that may include: > > - off: as in previous versions, no checking, just assume all pristines > are present > - pristines-on-demand: fetch "wanted" pristines; discard others > - fetch-only: fetch any pristine that's absent; do not discard I think, whatever the name of the property here, "off" is confusing for wanting all pristines (the back-compat / old / default (?) behaviour). To me it sounds like I am setting the feature "do not fetch all pristines" to off, so "please fetch all pristines" with a double negation. Maybe we should go for something like: pristine-fetching = full (or "eager", or "all", i.e. default) | lazy (or "on-demand") Perhaps with a third option "lazy-keep" (like your "fetch-only"), indicating on-demand, but not immediately cleaning it after use (don't know if this would be useful -- could be added later of course). Or "lazy-transient" for the "lazy with immediate cleaning after use" as opposed to "lazy" (which keeps fetched pristines once fetched). On Sat, Jan 29, 2022 at 9:22 AM Julian Foad wrote: > > Vincent Lefevre wrote: > >> [...] Specifying a pattern to match the WC path [or] per repository [...] > > > >But what if a WC can be accessed from different machines [...]? > > Then: > - The config option should be designed never to assume or depend on the > pristine store being in a particular state (such as fully populated). > - The user might want different behaviour on different machines, or the same > on all. > - The patch I posted yesterday in a separate thread allows the user to set > the config option in the user config or per-wc config. > - I noticed we already have some other config options in the '[working-copy]' > config section. We probably should allow the user to set those per-wc too. > - Julian Hmm. I think the pristine-fetching strategy that is chosen for a particular working copy should a property of that working copy. That's because it has a "persistent" impact on that working copy. Changing that strategy (if we would support that) severely impacts the disk layout of that particular working copy. It's not just a runtime thing, like using "exclusive sqllite locks" or some such (leaves no trace for the next user). If it would be a runtime setting, and Alice and Bob would both work on the same working copy, and the former has "pristine-fetching=full" and the latter "pristine-fetching=lazy" (or some detailed strategy with patterns, whatever), the working copy would be changed severely every time one or the other touches it. So I think the chosen pristine-fetching strategy for a working copy should be stored in the WC itself, probably in wc.db. However, we would still need related runtime config options. But I see them as "defaults" for when creating a new working copy. Perhaps these belong in a [working-copy defaults] or [working-copy creation] section, as opposed to the [working-copy] section which is more about runtime behaviour. Basically: - The chosen pristine-fetching strategy should be a persistent property of the WC, to be chosen at creation time. - Defaults for this should be part of our runtime-config area (and perhaps also options for 'svn checkout'). - We might introduce ways to change the setting of a given WC (but it's not a must have for the first iteration, I guess) On Fri, Jan 28, 2022 at 6:11 PM Evgeny Kotkov wrote: > > Julian Foad writes: > > > We could swap the scanning logic around to do the (quick) check for > > missing pristines before deciding whether a (slower) file "stat" is > > necessary. Positives: eliminates the redundant "stat" overhead which may > > be significant in working trees containing many files. Negatives: some > > re-work needed in the current implementation. > > > > Of these, the last one currently looks viable and useful. > > > > Does that one look promising to you? > > I might be missing something, but I don't yet see how we could save a stat(). > > Currently, a pristine is hydrated if and only if the corresponding working > file is modified. Let's say we check if a pristine is hydrated beforehand. > If we find out that pristine is dehydrated, we have to stat(), because if the > file is modified, then we need to hydrate. If we find out that pristine is > hydrated, we still have to stat(), because if the file is no longer modified, > then we need to dehydrate. What seems important to me is that, if a WC is set to pristine-fetching=full (or SVN 1.15 would be working with an older working copy, without the "pristine-fetching" property (say if upgrading of the wc format would not be needed)), that the code here can assume (like the old code) that the pristine is present. No need for an extra stat, just assume it is there, if not, error out like today (incidentally, this is a very annoying error to run into (if a pristine is accidentally deleted for some
Re: A two-part vision for Subversion and large binary objects.
On Fri, Jan 21, 2022 at 5:56 AM Karl Fogel wrote: > > On 20 Jan 2022, Julian Foad wrote: > >The more I think about this, the more I think we are prematurely > >complicating the requirements in this respect. I'm going to > >back-track > >and posit that a simple per-WC switch should suffice for the vast > >majority of cases, and has the benefit of simplicity. (The user > >might > >wish to set this based on the repository location -- local/fast > >versus remote/slow.) > > Personally I'd be very happy to start with this. We can always > improve the client-side UI for the feature more in the future. > > >I will note that I previously misunderstood the current > >'pristines-on-demand' implementation as fetching the pristine > >before a > >diff (for example) and discarding it afterwards. In fact it > >keeps the > >pristine as long as the file in question remains in a locally > >modified > >state, and only discards the pristine when (before or after some > >client > >operation) the file is no longer in a modified state. That is to > >say, it > >fetches pristines less often than I had thought. > > And it only fetches pristine for commands that absolutely need > pristine, I assume? (I think you said earlier that it does not > fetch for 'commit'.) > > I like the trick of keeping the pristine, once fetched, for as > long as the file is locally modified. > > >The only case in which a simple per-WC setting might be > >unsatisfactory > >is the following combination: > > > > - the repository is "slow" (and/or offline working is > > required); > > > > - and, in a single WC: > >- the WC data set is "huge" (relative to local disk space) in > >total; and > >- there is a subset of files on which the user needs to work > >(requiring diffs, etc.) often enough that fetching their > >pristines "on > >demand" is a problem; and > >- that subset of files is not "huge" in total; and > >- that subset of files can be distinguished from the rest by > >metadata. > > > >That is certainly a possible case, but we have no suggestion that > >it is > >at all common. It is not one of the cases driving this > >feature. So I > >think it is not something to design for at this stage. > > Well, that case is almost exactly our use case at my company :-), > except that I think fetching pristines on demand will be fine. > Thus, we can live with a per-WC setting. > > >I'm going to work on getting something more basic (per-WC yes/no) > >closer > >to production-ready and then we can re-assess it. > > Sounds good! > > Best regards, > -Karl I like where this is going. Thanks to all involved for pushing it forward :-). Just as another data point, at my company we have a slightly different use case where this feature would be great (I think), and where a simple per-WC-yes/no switch would work fine. In this case, we'd probably also want a "system-wide runtime config area default". The use case: we have a couple of build machines for "official full builds" (for test, staging, production, ... different stages) of our major applications. To save time, for most big applications, we keep re-using the large checked-out working copies (update, build, and switch to the next release branch if there is a next release). We also use those same working copies for backporting cherrypick merges to our release branches (it's all part of our release process, supported by build scripts and procedures to do these things). So, currently, our largest build server has 400 such working copies on disk, and they remain there "dormant" until someone updates, builds, cherrypick-merges, commits-a-tag-from-WC, ... Totalling around 450 GB at the moment. Not an absolute killer, but this is a growing number, and our sysadmins would be quite happy to see that number be +/- divided by 2 :-). Besides, these build servers are located in our data centers, "right next to" the SVN server, i.e. having a 1 Gb or 10 Gb connection to the repository. A perfect fit for "I don't need 99.9% of those pristines most of the time, and it's blazingly fast to get them when needed". Ideally, after pristines-on-demand become possible, I'd do the following: - Set a system-wide flag to make pristines-on-demand the default for new WC's. - Run a script to "convert" all existing working copies to pristines-on-demand (setting the per-WC flag). - Run a script to "vacuum-pristines" those converted working copies. - Receive chocolates from our sysadmins (probably not, but I can try). (BTW: a lot of those working copies are similar, so a feature to have a "shared pristine store" would also help, but that's another feature altogether, and perhaps much more difficult to get right -- both features would solve the wasted-disk-storage problem here, so I'm happy either way) Kind regards, -- Johan
Re: New release
On Tue, Jan 11, 2022 at 5:12 PM Daniel Sahlberg wrote: > Den tis 11 jan. 2022 15:34Julian Foad skrev: >> >> Johan Corveleyn wrote: >> > Thanks for starting to get this ball rolling, Daniel. >> >> Seconded. >> >> >> I have been pondering the question of which release(s) and in what >> >> order. I think 1.14.2 and 1.10.8 first, then 1.15.x later. >> >> Sounds good. >> >> As mentioned in another thread just now, I'm now able to work on issue #525 >> (pristines-on-demand) so we might consider if it's going to fit into the >> 1.15.0 time scale. I'm just starting today so if we could consider this in a >> few days once we've got some summary/plan/idea of how it might progress that >> would be good. > > I see several votes for releasing 1.15 "later". Nathan (I think) at one point > suggested that 1.15 might be "the performance release" with the "streaming > checkouts" from last summer so I think it makes sense to also bring this to > completion. So: Later = +1 for me. > >> >> As for release management, I can volunteer at least a little support to a >> release manager, based on what I learnt and can remember from doing it a few >> times in the last few years. > > > Agree it is good if more hands know the skills. For me January and February > are quite busy and I hope we can find someone to do 1.10.8 and 1.14.2 during > that timeframe. I might be able to free some time for a 1.15 release in > March/April. In principle I could volunteer for RM, but there are at least 3 things holding me back: 1. Lack of time, but I guess that goes for a lot of us around here. 2. I'm on Windows, and AFAIU our current release process requires *nix (at least our docs refer to a lot of unix-specific build tools that are needed [1]). I could in theory do all those things on another machine, for instance our own svn-qavm (though I guess I'd need enough permissions on such a machine to install tools etc), or set up a temporary (virtual?) machine on my own, but I'm not sure I want to go through all the hassle (given my previous point). And given the previous point, I certainly don't have the time to investigate and update our release process to make it work on Windows (though that would definitely be useful). 3. I'm on Windows, and am usually the only one who provides a binding release-vote for the Windows platform, and traditionally we don't count the RM's own vote among the three required (and at least one per major platform [2]). I know that's not really required (the RM's vote is just as valid as any other from a PMC member), but we seem to have been able to do it this way until now (I think). I suppose number 3 is not a very big issue, so let's say I have 2.5 reasons :-). Number 2 (unix required) is the big one. [1] https://subversion.apache.org/docs/community-guide/releasing.html#before-release [2] https://subversion.apache.org/docs/community-guide/releasing.html#releasing-votes -- Johan
Re: New release
Thanks for starting to get this ball rolling, Daniel. On Thu, Jan 6, 2022 at 6:41 AM Nathan Hartman wrote: ... > I have been pondering the question of which release(s) and in what > order. I think 1.14.2 and 1.10.8 first, then 1.15.x later. +1 > Regarding drumming up support for the backports: I will make an effort > to review items and look for other good candidates that aren't > nominated yet. I'll try to take some closer looks too. -- Johan
Re: Jira issues cleanup
Late to the party, but: thanks for doing this. Some more below. On Tue, Dec 28, 2021 at 12:20 PM Daniel Sahlberg wrote: > > Thanks Nathan for the quick review! > > I'm going to continue to add to this mail to have "one mail to rule them all" > and add more issues to the end as I go along. Please feel free to clean up > the mail when replying. > > Den tis 28 dec. 2021 kl 01:50 skrev Nathan Hartman : >> >> > SVN-4848: ARM BUILD ERROR >> > SVN-3007: perl bindings segfault very often >> > SVN-3609: Assertion in svn_path_is_canonical, svn_path_join with 'file:' >> > URL >> > SVN-4578: svnadmin pack cannot delete files during packing >> > SVN-4447: Error Conflict 409 > > > I've closed the ones above. +1 >> >> > SVN-4639: svn.exe causes ACCESS_VIOLATION >> > Reported in 2016, no comments, not discussed in the mailing list. >> > Relatively sparse on data, I've tested using a trunk build under Windows >> > 11 and "works for me". >> > * Close as cannot reproduce >> >> I haven't looked at this one in detail yet; gotta run... I'll try to >> come back to it. I agree you can close this as "cannot reproduce". > > SVN-667 handle file name case sensitivity edge cases > I know almost nothing about OSX file systems but I think the more modern ones > are case sensitive. This leaves win32, where (I believe) all file systems are > case insensitive but most are case preserving. According to my tests it > behaves much better than originally described. If someone on OSX could test > we should probably close this and add a new issue describing the current > state. > * Close as ... Later? Yeah, I'd say that issue is 99% solved since 1.7. The "case-only rename on Windows" problem was fixed in issue https://issues.apache.org/jira/browse/SVN-3702 (I think this fix also applies to case-insensitive filesystems on MacOS, though I have not tested that (but then again, perhaps case-insensitive filesystems on MacOS are not widely used anymore)). I'd say: let's close SVN-667 as "resolved", and add an issue link to SVN-3702. > SVN-3694 SWIG bindings, tempfiles cleanup > A script in Perl is using SWIG bindings. Filed in 2010 and there is a comment > by Roderich Schupp in 2015 "This is not a bug, it's a user error" suggesting > to that it is based on incorrect understanding of the SVN pools. According to > my test, I can reproduce the original issue but if I update the script > according to Roderich Schupp' suggestion, the error goes away. > * Close as Invalid +1 > SVN-3398 Seg fault with BDB repos > Close, since BDB is deprecated since Subversion 1.8. > * Close as Won't fix +1 > SVN-3494 moving a branch stumps svnmerge.py > Very sparse on details. No mentioning of the mailing list (but I didn't > search the archives. > * Close as Invalid +1, or as "won't fix", because svnmerge.py is part of contrib, and contrib is not supported anymore (not by the Subversion community anyway) -- and in any case svnmerge.py is deprecated I think. IIRC, svnmerge.py was a script that did some form of merge tracking, before the core implementation arrived in Subverison 1.5. Out of curiosity I've taken a quick look in its documentation: the README (https://svn.apache.org/repos/asf/subversion/trunk/contrib/client-side/svnmerge/svnmerge.README) refers to a wiki page that still exists: https://www.orcaware.com/svn/wiki/Svnmerge.py. That page contains a FAQ entry: [[[ How do I migrate from svnmerge.py to Subversion 1.5's Merge Tracking? Use svnmerge-migrate-history.py to convert the merge history written by svnmerge.py into Subversion 1.5's format. ]]] In any case I think it's pretty OK to close all outstanding issues about svnmerge.py. > SVN-2802 Tests failing to cleanup bdb repositories over ra_neon > BDB and Neon are both deprecated. > * Close as Won't fix +1 > * SVN-3276 Access violation (svn move, using some java client) > Reported in 2008 on 1.5.x and there is a comment that it is not directly > using the javahl bindings but a higher level client. Nothing happened since > 2010. > * Close as Invalid +1 > * SVN-2635 svnsync pre-revprop-change hook failed > An error occured when using a batch file as pre-revprop-change hook script. > Problem went away when using a compiled program. Lesson learned: Write proper > scripts. > * Close as Invalid +0. I'm not sure about this one (maybe as "cannot reproduce", but I haven't tried to reproduce it myself). I think using BAT for hook scripts on Windows should be fine in general, so it's not clear to me why this failed. But it's oh so fragile and difficult. Maybe there is some quoting issue or something similar. Hard to say. Perhaps, to be able to make the decision for closing it with "cannot reproduce", someone might need to try the example the user gave in: https://issues.apache.org/jira/browse/SVN-2635?focusedCommentId=14923562=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14923562 > * SVN-3534 Failed ra_neon commit without --no-unlock results in dead
Re: A strong WTF on compiling out plaintext password support by default?!
On Sun, Oct 3, 2021 at 12:39 PM Daniel Sahlberg wrote: > > Hi, > > I would like to reboot this thread once again. I have read through all messages and I have tried to make a summary of the important points. The date/time references are as seen in https://mail-archives.apache.org/mod_mbox/subversion-dev/. > > There are numerous descriptions of problems with the different keyrings. I think the most important are: > - Some requiring GUI boxes to unlock, even showing GUI boxes on the local display while running over SSH > - No easy way to work with the keyrings non-interactive > > The original request was to find some way to store credentials in the plaintext cache. > > Two different solutions have been presented: > - Reverting the compile time default to enable the plaintext password store, while setting runtime configuration options to disable it, following the example set by OpenBSD. > Four persons seem to have expressed support for this idea (Mark Phippard, Johan Corveleyn, myself (2021-08-26 13:34 to 2021-08-27 07:56) and Martin Edgar Furter Rathod (2021-08-31 12:46)). The former three prefering it over the svn auth, but accepting the idea of svn auth. Martin suggested to move the plaintext support to a separate library that could be installed separately (or removed after installation), no-one picked up this idea. > > - Adding an svn auth add command > One person seem to have expressed support for this (Stefan Sperling (2021-08-24 08:27 and again 2021-08-27 12:40)). The first message is the earliest suggestion of svn auth add that I can find. In the latter message, Stefan clarifies that he belives svn auth add is the better design but wouldn't stand in the way of consensus. He also wrote he had been willing to invest time in writing the code for svn auth add but not for switching the default compile-time behaviour. > Regarding the need to validate the credentials with the server, it seems this was based on pseudocode by Johan Corveleyn (2021-08-24 14:25) doing svn ls, and then asking for a new password if svn ls failed. It was mentioned by Daniel Shahaf that svn ls might succeed even if the credentials were incorrect (as with svn.apache.org offering anonymous access) or that the check only verified "r" access and later code might require "w" access. > It seems to be a general agreement that it is currently not possible to validate if some credentials have a certain level of access to a certain path, in a general case. It was suggested to add RA API calls, but these would only work with new versions of both the server and the client. > There has been a significant discussion whether to verify the credentials and whose responsibility it is to store correct credentials (with appropriate access). > > This is the end of the summary. Below are my personal reflections > > I have ignored the discussion regarding rethorics since I hope we can continue without it. If someone feels hurt because of the discussion I hope it can be worked out without affecting the future discussion in this thread. I think all contributors have come with valuable input on the design questions so far. > > It seems most messages are concerning the svn auth add solution. To add this command without contacting the server seems to be quite a bit of code but not impossible. It would also be a client-side only solution not requiring any specific server version. To add support on the server side might be difficult, especially considering that (part of) the authentication/authorization might be done outside of the Subversion code. > > The decision to change the compile time default was made in 2018-10-31 within less than 12 hours and without much debate. It was committed less than 19 hours after the initial message. > I'm assume the impact of an incorrect password in the store is application dependant but a password can grow stale even if it was correct at the time of writing so the administrator would have to manage this anyhow. From my point of view, verifying the credentials is nice but not necessary to solve the initial use case. > > Until we completely remove the possibility to read the plaintext store security conscious organisation might have issues. Having svn auth add as an official command makes it more obvious that this possibility exists - instead of users running unofficial scripts or even manually editing the config files. Anyone who feel it should not even read the plaintext password store could convince their vendor to remove it or roll their own. > > Below is my understanding of the decision tree and my proposal: > > The first point where we must reach agreement is whether or not to do anything at all: > * There has been several complaints from several different users over >the last years who's workflow has been interrupted. There may be users >or organisations who dispro
Re: SVN 1.14.1 slightly broken on Windows for working copies in drive root?
On Thu, Sep 30, 2021 at 11:24 AM Thomas Singer
wrote:
>
> Hello,
>
> I have a working copy of the SVN repository at D:\src\svn. With SVN
> 1.14.1 binaries I'm getting following status output
>
> D:\src\svn>svn.exe status -u
> Status against revision: 1893746
>
> When I create a 'substed' drive G: for that directory using
>
> D:\src\svn>subst g: D:\src\svn
>
> D:\src\svn>g:
>
> I'm getting following output (note the exclamation mark):
>
> G:\>C:\temp\svn\svn.exe status -u
> ! 1892650 .
> Status against revision: 1893746
>
> Is this a known/already fixed regression?
Yes, this is a known issue. However it's not a bug in Subversion, but
in APR 1.7.x. If you use APR 1.6.5 you'll see the problem will no
longer occur.
It has been reported here some time ago ("Subversion reports status
fault on substituted drive"):
https://lists.apache.org/thread.html/4797303a065e9c1091bbf445acaa3121ccb27ac2d661eb0debd1cf40%40%3Cdev.subversion.apache.org%3E
It has also been brought to the dev@apr list, where it has been
discussed somewhat, but so far without conclusion:
https://lists.apache.org/thread.html/r28e478074055436b27b13f7487203448079aea5adfe4207007ad7ea9%40%3Cdev.apr.apache.org%3E
--
Johan
Re: Subversion on Solaris (was Re: A strong WTF on compiling out plaintext password support by default?!)
On Thu, Sep 2, 2021 at 7:29 AM Daniel Sahlberg wrote: > > Den mån 23 aug. 2021 kl 12:15 skrev Johan Corveleyn : >> >> Anyway, concerning package maintainers, for Solaris, I'm getting even >> more depressed ... >> * We were using Collab.net's distro, but apparently their Solaris >> build is no longer maintained: >> https://www.collab.net/downloads/subversion >> * Wandisco then? Nope: >> https://www.wandisco.com/source-code-management/subversion#solaris ... >> only Windows, Linux and MacOS. >> * Fortunately, there is still a relatively recent build on OpenCSW: >> https://www.opencsw.org/packages/subversion/ > > > I have been working with Dagobert Michelsen of OpenCSW to build Subversion > 1.14.1 (mailing list thread starting at > http://lists.opencsw.org/pipermail/users/2021-August/010493.html). > > Due to my relative inexperience with Solaris I have not been able to compile > Subversion myself so I have had some problems to execute the complete test > suite but I have run the majority of tests and it all seems fine. The failing > tests are because some missing tooling and not because of errors in the > Subversion build. > >> >> (-> Wandisco's link should be removed from >> http://subversion.apache.org/packages.html#solaris -- making that >> section empty ... or should we add a link to opencsw then?) >> (-> this abandoning of packagers / maintainers makes me feel our >> ecosystem is breaking down) > > > This was implemented in the main site as r1892681 > >> So I guess my best shot is contacting the maintainer of the openCSW >> package, and asking him to add the --enable-plaintext-password-storage >> configure flag and make a new build then. > > > I have raised this question, see mailing list thread above. > > Kind regards, > Daniel Thanks Daniel. You're a star! :-) -- Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Fri, Aug 27, 2021 at 2:03 PM Daniel Shahaf wrote: > > Consensus can only result from an open discussion. That's a standard > ASF operating principle. > > The rhetoric in this thread effects chill on anyone who has an opinion > different from the opinion of certain speakers. I must say I have not seen / felt any rhetoric. Only on-topic arguments pro and contra certain approaches. So I'm a bit lost here. I rebooted this thread last Monday after it lay low for a week or so. Not because I wanted to start an argument, but mainly to express my frustration over this issue *as an svn admin* (because I was investigating, in our company, the upgrade to a more recent svn client on a build server -- which I have since postponed, possibly forever, mainly because of this issue). I was happy to see so many people weighing in with their thoughts and suggestions. As far as I could see, this was a productive thread. > Therefore, this thread _cannot_ consense at this time. > > I think we should (US)table the matter for a few weeks and then (UK)table > it again. Np from my part, we can (US)table it for a while. For me, there is no urgency. We'll just not upgrade svn on our build machine for the time being (no rush). But of course, there are others (including the OP of this thread) who might be interested in a solution. Anyway, what's a couple of weeks in an svn release cycle ;-) ... -- Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Thu, Aug 26, 2021 at 3:44 PM Mark Phippard wrote: > > On Thu, Aug 26, 2021 at 6:30 AM Stefan Sperling wrote: > > > The answer might be that 'svn authz add' should simply not contact the > > server to check credentials. Which means we cannot check upfront whether > > the user running 'svn auth add' knows valid credentials. > > Yeah that seems reasonable. Basically an "official" version of what > the scripts are doing. > > All that said, I still favor restoring the previous behavior where > this is enabled by default and a packager can compile it out if they > so choose. +1, I would prefer that as well (was quite happy with the previous behavior :-)). If that's not an option (because of $security-reasons / loud voices), a workaround with some 'svn auth' extension would definitely help mitigate the break in usability / backwards compatibility that now occurred. -- Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Thu, Aug 26, 2021 at 4:31 PM Daniel Shahaf wrote: > > Johan Corveleyn wrote on Thu, 26 Aug 2021 12:41 +00:00: > > On Wed, Aug 25, 2021 at 8:52 PM Daniel Shahaf > > wrote: > > > This thread is on dev@ as opposed to users@, so I'm trying to solve the > > > problem generically, rather than just your specific $WORK scenario. > > > > I get the feeling I'm missing something, but I still don't understand > > what authz has to do with the problem at hand here (i.e. detecting > > expired passwords so we can ask the user for the new one). > > Your problem statement is "Replace cached passwords that are expired". > > I'm solving a more general problem statement, "Replace cached passwords > that can't be used to commit with", regardless of why. Okay, sure, but that's another question than what we started with. BTW, I don't really follow how you can replace just a cached password for getting "write access". Doesn't "upgrading from read-only to read-write access" also imply using another username? Or can I have two passwords for one user, where one gives me read-only access and one gives me write access? I.e. shouldn't the more general problem statement be "Replace cached username+passwords that can't be used to commit with"? (hence my first response with "huh, shouldn't a --username make that problem moot"? I.e. the user knows which user he's willing to connect with, and he knows the authz rules, he just wants the password to be checked and "re-cached" if need be) -- Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Wed, Aug 25, 2021 at 8:52 PM Daniel Shahaf wrote:
> Johan Corveleyn wrote on Wed, 25 Aug 2021 07:16 +00:00:
> > On Tue, Aug 24, 2021 at 7:03 PM Daniel Shahaf
> > wrote:
> > > Johan Corveleyn wrote on Tue, 24 Aug 2021 15:22 +00:00:
> > > > On Tue, Aug 24, 2021 at 4:45 PM Daniel Shahaf
> > > > wrote:
> > > > > Johan Corveleyn wrote on Tue, 24 Aug 2021 14:25 +00:00:
> > > > > > OTOH, if this kind of behaviour is too far-fetched for a single
> > > > > > subcommand, I might be able to do it by invoking two commands, if I
> > > > > > could succesfully (and invisibly) detect that a cached password is
> > > > > > no
> > > > > > longer correct. As in:
> > > > > >
> > > > > > svn ls --non-interactive $URL >/dev/null
> > > > > > # if exit-code != 0, parse error code to see if it indicates
> > > > > > "auth failed"
> > > > > > if ("auth failed"):
> > > > > > svn auth add $URL
> > > > > >
> > > > >
> > > > > What happens if a valid username/password are cached that have read-
> > > > > only access and one wants to preseed a username/password that have
> > > > > read-
> > > > > write access?
> > > >
> > > > I don't know. We don't have that use case at $company, trying to keep
> > > > things simple :-).
> > > >
> > > > Ah but shouldn't 'svn auth' carry an optional --username parameter? In
> > > > that case, I don't see the problem, I guess.
> > >
> > > My point here is that that pseudocode doesn't handle ensuring that the
> > > cached credentials have read-write access. Existence of «svn auth
> > > --username»
> > > won't change that, because the «svn auth» call is inside an if() block
> > > whose condition will false negative.
> > >
> > > Is there a way to test whether one has rw access without actually doing
> > > a commit or a revprop edit? It's possible with hooks, of course, but is
> > > it also possible without hooks?
> >
> > I'm not sure I understand: why would I need to know that the cached
> > credentials have read-write access?
> >
>
> Your pseudocode detects credentials that have no read access, whether
> for lack of authz or for failing to authn.
Oh? I was assuming that we would be able to discern failed authn (as
in "incorrect username or password") from failed authz (as in "403
Forbidden"). Is that not the case? Do both failed authn and failed
authz yield the same error code / message?
I'm only interested in failed authn, because this entire thread is
about the disabling of the plaintext password cache, and I just want
to know if the cached password is now invalid (with failed authn) so I
have to ask the user for a new one (to potentially use another tool to
inject that new password into the plaintext cache).
> I was asking how to
> generalize this to detecting credentials that successfully authn and
> authz but only for read-only access rather than for full access. That
> would be necessary if the "preseeded" credentials are to be used for
> a write operation (a commit or a revprop edit).
>
> > I only want to verify the authentication part,
>
> This thread is on dev@ as opposed to users@, so I'm trying to solve the
> problem generically, rather than just your specific $WORK scenario.
I get the feeling I'm missing something, but I still don't understand
what authz has to do with the problem at hand here (i.e. detecting
expired passwords so we can ask the user for the new one).
--
Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Tue, Aug 24, 2021 at 7:03 PM Daniel Shahaf wrote:
> Johan Corveleyn wrote on Tue, 24 Aug 2021 15:22 +00:00:
> > On Tue, Aug 24, 2021 at 4:45 PM Daniel Shahaf
> > wrote:
> > > Johan Corveleyn wrote on Tue, 24 Aug 2021 14:25 +00:00:
> > > > OTOH, if this kind of behaviour is too far-fetched for a single
> > > > subcommand, I might be able to do it by invoking two commands, if I
> > > > could succesfully (and invisibly) detect that a cached password is no
> > > > longer correct. As in:
> > > >
> > > > svn ls --non-interactive $URL >/dev/null
> > > > # if exit-code != 0, parse error code to see if it indicates "auth
> > > > failed"
> > > > if ("auth failed"):
> > > > svn auth add $URL
> > > >
> > >
> > > What happens if a valid username/password are cached that have read-
> > > only access and one wants to preseed a username/password that have read-
> > > write access?
> >
> > I don't know. We don't have that use case at $company, trying to keep
> > things simple :-).
> >
> > Ah but shouldn't 'svn auth' carry an optional --username parameter? In
> > that case, I don't see the problem, I guess.
>
> My point here is that that pseudocode doesn't handle ensuring that the
> cached credentials have read-write access. Existence of «svn auth --username»
> won't change that, because the «svn auth» call is inside an if() block
> whose condition will false negative.
>
> Is there a way to test whether one has rw access without actually doing
> a commit or a revprop edit? It's possible with hooks, of course, but is
> it also possible without hooks?
I'm not sure I understand: why would I need to know that the cached
credentials have read-write access?
I only want to verify the authentication part, and see if the password
that was cached is no longer valid (in which case we need to ask the
new one to the user). I don't really care at this point whether we're
talking about read-only or read-write access, just want to refresh the
password in the cache (for whatever username the user is using ...
it's his business to use the correct username for whatever use case
he's using here :-)).
--
Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Tue, Aug 24, 2021 at 4:45 PM Daniel Shahaf wrote:
>
> Johan Corveleyn wrote on Tue, 24 Aug 2021 14:25 +00:00:
> > OTOH, if this kind of behaviour is too far-fetched for a single
> > subcommand, I might be able to do it by invoking two commands, if I
> > could succesfully (and invisibly) detect that a cached password is no
> > longer correct. As in:
> >
> > svn ls --non-interactive $URL >/dev/null
> > # if exit-code != 0, parse error code to see if it indicates "auth
> > failed"
> > if ("auth failed"):
> > svn auth add $URL
> >
>
> What happens if a valid username/password are cached that have read-
> only access and one wants to preseed a username/password that have read-
> write access?
I don't know. We don't have that use case at $company, trying to keep
things simple :-).
Ah but shouldn't 'svn auth' carry an optional --username parameter? In
that case, I don't see the problem, I guess.
> > But then the burden is on me detecting the "auth failed" correctly,
> > and making sure it's the "password refused" kind of auth failure (I
> > guess there's an error code for that).
>
> There's an error code for that, but it's the E42 value in stderr,
> not the exit code. The exit code is generally 1 (unless segfault or
> something).
Yup, parsing the error code from stderr was what I was thinking of.
Not great, but it'd work.
--
Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Tue, Aug 24, 2021 at 1:24 PM Stefan Sperling wrote:
>
> On Tue, Aug 24, 2021 at 12:16:48PM +0200, Johan Corveleyn wrote:
> > But: obviously I have disabled, in the runtime config area, the
> > warning prompt that "Your password will be stored in plaintext" (I
> > have disabled it system-wide, in /etc/subversion). Yes, we know this
> > and we accept it. I would not like this nag screen to be forced with
> > no way for an admin to disable it. Ah, I might be able to write a
> > wrapper script to dev/null the warning :-). All this effort for ...
> > nothing (from my perspective).
> >
> > Perhaps a meta-itch here: the old behaviour has been there for 20
> > years, and then we decided to change this. In the
> > super-stable-mature-super-backwards-compatible part of our lifecycle.
> > Not the best timing I think. Perhaps this should have rather been on
> > the 2.0-wishlist ("Come up with a better pwd caching solution on
> > non-Windows platforms").
>
> OK, I see why my proposal as it is wouldn't help you.
> But if we tweak it slightly, it could work?
>
> We could make 'svn auth add' only ask for a password if no valid
> password can be found in the cache. The command would first contact
> the server and if it manages to authenticate it would do nothing else.
Thanks for thinking along, Stefan :-).
Yes, that might work. But perhaps 'add' wouldn't be a great name for
this kind of behaviour. Maybe something like 'svn auth cache-validate
$URL' or 'cache-freshen' or something like that.
OTOH, if this kind of behaviour is too far-fetched for a single
subcommand, I might be able to do it by invoking two commands, if I
could succesfully (and invisibly) detect that a cached password is no
longer correct. As in:
svn ls --non-interactive $URL >/dev/null
# if exit-code != 0, parse error code to see if it indicates "auth failed"
if ("auth failed"):
svn auth add $URL
But then the burden is on me detecting the "auth failed" correctly,
and making sure it's the "password refused" kind of auth failure (I
guess there's an error code for that).
> And we could avoid showing a plaintext prompt. Instead we could require
> a --plaintext command line option to allow storage in plaintext.
> Without --plaintext, 'svn auth add' would fail if the password cannot
> be stored encrypted and general plaintext-support was not enabled at
> compile time. The only interactive component presented to the user would
> be a password prompt.
Uhuh, sound sensible. Perhaps it should be '--allow-plaintext' or
something, so that, like now, it tries to use encrypted storage if
available, but is allowed to fallback to plaintext if there is nothing
else. Or do we rather want an option to force plaintext, even if there
is encrypted storage available (should that be named --force-plaintext
then?)? Dunno.
> Failing that, I agree that the best solution would be to simply
> revert the default setting of the compile-time switch back to 'yes'.
>
> This won't make the hardline "no plaintext, please" crowd very happy.
> But the middle ground, where all this can be configured at run-time
> probably won't satisfy them either. I guess that is why a compile-time
> switch exists in the first place.
To make things even more complicated, we could add another
compile-time switch (which would not be on by default!) to
compile-time disable support for the --allow-plaintext option to 'svn
auth add' :-). Companies / packagers that are extra sensitive can roll
their own then.
--
Johan
Re: A strong WTF on compiling out plaintext password support by default?!
[ sorry Robby, I left you out of a previous reply -- looping you back
in in case you're interested in this tangent ... ]
On Tue, Aug 24, 2021 at 10:34 AM Stefan Sperling wrote:
>
> On Mon, Aug 23, 2021 at 02:45:44PM +0200, Johan Corveleyn wrote:
> > Thanks, those are good efforts (and thanks to both Daniels for writing
> > them), but I'm afraid those workarounds are not good enough. The thing
> > is: this is not for me alone. This needs to be handled in buildscripts
> > that can be run by around 50 developers.
>
> One thing is unclear to me:
>
> At some point, someone has to run an svn command to get the passward
> cached on Solaris in the first place or build scripts would be failing.
>
> How is this done? Each of the 50 developers ran 'svn checkout' or
> something similar to get the password cached? Or does the password
> get cached in a non-interactive, automated way?
>
> The point of this question is to understand whether my 'svn auth add'
> proposal described elsewhere in this thread would solve your problem.
These buildscripts are run manually / interactively on this machine
(developers take turns for this weekly task, for different
applications). For most applications there is a canonical (shared)
working copy on this build-server, so they reuse that (or otherwise
they can set one up quickly with a script). The buildscripts are
written in ant (we're building Java applications actually).
The ant targets that do something with svn have a
"depends=svn-checkpwd" clause, which makes them first call the
"svn-checkpwd" target. This does a minimal password-requiring thing
with svn (I think it is something like 'svn ls https://svnserver/repo
>/dev/null'). If the cached password is still correct, user sees
nothing (carry on), if not they get a password prompt and type in the
new password.
I suppose I could rewrite that 'svn-checkpwd' target to use 'svn auth
add' or something similar, but I should also have a way to verify
whether the cached password is still valid (so I can get the same
behaviour as before: only bother the user if the cached password is no
longer valid).
But: obviously I have disabled, in the runtime config area, the
warning prompt that "Your password will be stored in plaintext" (I
have disabled it system-wide, in /etc/subversion). Yes, we know this
and we accept it. I would not like this nag screen to be forced with
no way for an admin to disable it. Ah, I might be able to write a
wrapper script to dev/null the warning :-). All this effort for ...
nothing (from my perspective).
Perhaps a meta-itch here: the old behaviour has been there for 20
years, and then we decided to change this. In the
super-stable-mature-super-backwards-compatible part of our lifecycle.
Not the best timing I think. Perhaps this should have rather been on
the 2.0-wishlist ("Come up with a better pwd caching solution on
non-Windows platforms").
--
Johan
Re: A strong WTF on compiling out plaintext password support by default?!
On Mon, Aug 23, 2021 at 1:50 PM Nathan Hartman wrote: > > On Mon, Aug 23, 2021 at 6:15 AM Johan Corveleyn wrote: >> >> On Fri, Aug 7, 2020 at 7:01 AM Robby Zinchak wrote: >> > >> > Small rant here from a very long time subversion user, regarding >> > subversion project's decision to compile out plaintext password storage by >> > default (https://marc.info/?l=subversion-commits=154101482302608=2). >> > >> > There are a tremendous number of scenarios where users would desire to use >> > subversion without a keyring -- for me, that's today running Ubuntu 20.04 >> > and trying to set up an automated subversion client on a server VM. I >> > obviously can't be logging into a keyring every time the server reboots, >> > that'd be idiotic. >> > >> > I cannot fathom how the team thought this was a good decision. It reeks >> > of devs thinking "we know better, force the users to do it this way," >> > without actually understanding the needs of your users. >> > >> > I'm left with four solutions as far as I can see it: >> > >> > 1) Crowbarring one of the supported keyrings into not needing a keyring >> > password. Assuming this is even possible, it seems like a lot of extra >> > work with no benefit, and added fragility. I am loathe to dive into a >> > whole set of docs to try and figure this out (assuming it's even >> > possible), when the old methods worked just fine. >> > >> > 2) Compiling my own subversion with the enable-plaintext-password-storage >> > flag -- obviously insecure since there's no way I'll be able to keep up >> > with software updates. And I've heard it's quite difficult to compile >> > subversion, so that'll waste precious time I could be spending on >> > something else that's actually productive for my business. >> > >> > 3) Finding an ubuntu package overlay by a third party, questionably >> > insecure since now I have to trust an unofficial/unvetted third party with >> > providing svn builds. >> > >> > 4) Bite the bullet and just switch to another VCS >> > >> > Every time version control comes up in dev conversations among my peers, I >> > go to great lengths to defend SVN against the many criticisms my peers >> > level at it and promote it to other devs looking for a quality VCS. But >> > honestly this decision is one of the most myopic ones I've seen in years >> > on any software project and reeks of the project developers making an >> > idealistic stand that inconveniences users with no practical real-world >> > benefit. The decision should be left to the user, rather than forcing >> > them into a difficult situation. The earlier change to make plaintext >> > something users have to intentionally opt into makes total sense so that >> > users are aware of what they're opting into - but removing it entirely is >> > too far. I guarantee you, right now there are people trying to puzzle >> > through this stack overflow answer, giving up, and switching to git. >> > (https://stackoverflow.com/questions/2899209/how-to-save-password-when-using-subversion-from-the-console) >> > This is a downright awful decision for the overall long term health of >> > what's left of the subversion userbase. >> > >> > I would love to hear what the expected workaround is for users running >> > automated subversion clients on server VMs, because all the options look >> > rather terrible. >> > >> > Thanks for listening to my rant, and be assured it comes only from a place >> > of wanting to see subversion succeed. >> > >> > Best, >> > Robby >> >> Late to te party, but I have to agree with Robby here. I'm only >> running into this now, since we'd like to upgrade the 1.9 client on >> our Solaris build machine to a more recent version ... sigh. >> >> I know the decision to disable plaintext pwd storage by default was >> briefly discussed on this very list [1], but sadly I didn't pay >> attention back then. I have a lot of respect for all people involved >> here, but I think this was a mistake, especially WRT server machines >> which don't have a GUI, no X11 etc. Or even if they have it installed, >> why force additional work on users / sysadmins that have been running >> these machines for years, and now have to jump through additional >> hoops, even if they decided before (through explicit configuration) >> that they were OK with
Re: A strong WTF on compiling out plaintext password support by default?!
On Fri, Aug 7, 2020 at 7:01 AM Robby Zinchak wrote: > > Small rant here from a very long time subversion user, regarding subversion > project's decision to compile out plaintext password storage by default > (https://marc.info/?l=subversion-commits=154101482302608=2). > > There are a tremendous number of scenarios where users would desire to use > subversion without a keyring -- for me, that's today running Ubuntu 20.04 and > trying to set up an automated subversion client on a server VM. I obviously > can't be logging into a keyring every time the server reboots, that'd be > idiotic. > > I cannot fathom how the team thought this was a good decision. It reeks of > devs thinking "we know better, force the users to do it this way," without > actually understanding the needs of your users. > > I'm left with four solutions as far as I can see it: > > 1) Crowbarring one of the supported keyrings into not needing a keyring > password. Assuming this is even possible, it seems like a lot of extra work > with no benefit, and added fragility. I am loathe to dive into a whole set > of docs to try and figure this out (assuming it's even possible), when the > old methods worked just fine. > > 2) Compiling my own subversion with the enable-plaintext-password-storage > flag -- obviously insecure since there's no way I'll be able to keep up with > software updates. And I've heard it's quite difficult to compile subversion, > so that'll waste precious time I could be spending on something else that's > actually productive for my business. > > 3) Finding an ubuntu package overlay by a third party, questionably insecure > since now I have to trust an unofficial/unvetted third party with providing > svn builds. > > 4) Bite the bullet and just switch to another VCS > > Every time version control comes up in dev conversations among my peers, I go > to great lengths to defend SVN against the many criticisms my peers level at > it and promote it to other devs looking for a quality VCS. But honestly this > decision is one of the most myopic ones I've seen in years on any software > project and reeks of the project developers making an idealistic stand that > inconveniences users with no practical real-world benefit. The decision > should be left to the user, rather than forcing them into a difficult > situation. The earlier change to make plaintext something users have to > intentionally opt into makes total sense so that users are aware of what > they're opting into - but removing it entirely is too far. I guarantee you, > right now there are people trying to puzzle through this stack overflow > answer, giving up, and switching to git. > (https://stackoverflow.com/questions/2899209/how-to-save-password-when-using-subversion-from-the-console) > This is a downright awful decision for the overall long term health of > what's left of the subversion userbase. > > I would love to hear what the expected workaround is for users running > automated subversion clients on server VMs, because all the options look > rather terrible. > > Thanks for listening to my rant, and be assured it comes only from a place of > wanting to see subversion succeed. > > Best, > Robby Late to te party, but I have to agree with Robby here. I'm only running into this now, since we'd like to upgrade the 1.9 client on our Solaris build machine to a more recent version ... sigh. I know the decision to disable plaintext pwd storage by default was briefly discussed on this very list [1], but sadly I didn't pay attention back then. I have a lot of respect for all people involved here, but I think this was a mistake, especially WRT server machines which don't have a GUI, no X11 etc. Or even if they have it installed, why force additional work on users / sysadmins that have been running these machines for years, and now have to jump through additional hoops, even if they decided before (through explicit configuration) that they were OK with plaintext password storage (= their decision / responsability). I have been defending Subversion at my company for a long time, highlighting its stability, simplicity and care for *backwards compatibility*. Unfortunately, backwards compatibility is broken here (I understand security can trump backwards compatibility, but to me this feels more like a "let's make this even more inconvenient" kind of security; not plugging a glaring security hole). So there goes one of my most important arguments pro SVN :-(. I know Stefan Sperling suggested in a reply to convince package maintainers, if necessary, to re-enable it with a configure flag (as he did for OpenBSD). But isn't that world upside down? Should every packager now explicitly enable it? Why not leave the default as it was, and tell package maintainers to disable it if they feel sensitive about this? Anyway, concerning package maintainers, for Solaris, I'm getting even more depressed ... * We were using Collab.net's distro, but
Re: [PATCH] limit diff effort to fix performance issue
On Tue, Jun 8, 2021 at 3:29 PM Nathan Hartman wrote: > > On Tue, Jun 8, 2021 at 5:55 AM Stefan Sperling wrote: > > > > On Tue, Jun 08, 2021 at 01:45:00AM -0400, Nathan Hartman wrote: > > > In order to do some testing, I needed some test data that reproduces > > > the issue; since stsp can't share the customer's 100MB XML file, and > > > we'd probably want other inputs or sizes anyway, I wrote a program > > > that attempts to generate such a thing. I'm attaching that program... > > > > > > To build, rename to .c extension and, e.g., > > > $ gcc gen_diff_test_data.c -o gen_diff_test_data > > > > > > To run it, provide two parameters: > > > > > > The first is a 'seed' value like you'd provide to a pseudo random > > > number generator at init time. > > > > > > The second is a 'length' parameter that says how long (approximately) > > > you want the output data to be. (The program nearly always overshoots > > > this by a small amount.) > > > > > > Rather than using the system's pseudo random number generator, this > > > program includes its own implementation to ensure that users on any > > > system can get the same results when using the same parameters. So if > > > different people want to test with the same sets of input, you only > > > have to share 2 numbers, rather than send each other files >100MB of > > > useless junk. > > > > > > Example: Generate two files of approx 100 MB, containing lots of > > > differences and diff them: > > > > > > $ gen_diff_test_data 98 100m > one.txt > > > $ gen_diff_test_data 99 100m > two.txt > > > $ time diff one.txt two.txt > /dev/null > > > > > > With the above parameters, it takes my system's diff about 50 seconds > > > to come up with something that looks reasonable at a glance; svn's > > > diff has been crunching away for a while now... > > > > Thank you Nathan, this is incredibly useful! > > > > Would you consider committing this tool to our repository, e.g. somewhere > > within the tools/dev/ subtree? > > > Sure, done in r1890601. > > It's in tools/dev/gen-test-data/gen_diff_test_data.c. > > I added the gen-test-data directory in case we want to add other > sample data generators in the future. As for test data, I just remembered something: in 2015 Bert developed a tool called "AnonymizedFileDumper", after some discussions we had during a hackathon and on IRC (related to blame and diff performance). With this tool one can create a dump file from (part of) a repository, with all text lines replaced by their CRC32 checksum (so identical lines remain identical, but other than that the actual information is mostly gone). If you combine this with svndumpfilter for stripping out the log messages and the author names, I think it's pretty much stripped of all sensitive information (I remember I asked Bert to add 'eliding autor names and log messages' as extra features of his dumper tool, but don't remember whether he eventually got around to that). It is the perfect tool for creating test data out of real repositories out there, without leaking company data, so other devs can take a look. I don't have the tool handy anymore, but I just searched the IRC logs and found some mentions of it here: https://colabti.org/irclogger/irclogger_log_search/svn-dev?search=nonymized=search=20150101-20151231=checked The binary download link still works, but it's a Windows binary. I don't know if the sources are still available from the sharpsvn repository. @Stefan: maybe you can use this to create an anonymized test case out of the 100 MB XML file of the elego client? You'd need a Windows machine if you want to use that binary from 2015. @Bert: WDYT? Are the sources still out there? Maybe this can be ported to "plain old svn test tools" (perhaps with the help of someone here that can spend some time on it)? (and hello by the way :-)) -- Johan
Re: [PATCH] limit diff effort to fix performance issue
On Tue, Jun 8, 2021 at 3:24 PM Stefan Sperling wrote: > > On Tue, Jun 08, 2021 at 02:57:34PM +0200, Johan Corveleyn wrote: > > Okay, I focused on the first revision causing the annotate to differ, > > and with some debug logging: > > - p went up to 139 > > - length[0]=1942, length[1]=1817 > > > > Now, 1942 lines on the left and 1817 on the right doesn't seem all > > that many (that's what remains after prefix-suffix stripping). I see > > no reason 'svn diff' shouldn't be able to calculate a minimal diff for > > those sizes in a reasonable amount of time. So if p can go up to such > > a "cost" for such "reasonable" lenghts, I imagine we should put the > > actual limit much higher. I suppose we can just as well set "min_cost" > > to 1024 or even higher. 64 is way too low. > > Thanks! > I have set the value back to at least 1024 with this new version of patch. Hmmm, apparently I'm still running into the limit. Even with 8192 I'm hitting it at least once. The actual diff there is not really pretty, but it's an actual manual change made by some developer years ago, and the "minimal diff" is more or less readable / understandable. The log message is something like "Group sections related to multimedia module", and it shuffles around a lot of xml sections to group them together. In this case, length[0]==length[1]==46780. Pretty big for the LCS, but not humongous. The value of 'p' goes up to 8279. With the limit set to 16384, I'm not hitting it, and the annotate comes out as with the original. Now, I'm a bit puzzled why limit==8192 already takes 18s in your tests. In my situation, with the above diff, I get: limit==8192: 3.3s limit==16384: 3.5s (that's including downloading both versions from the server over https) So I'm wondering why it takes so long in your case. One thing to keep in mind here is that, since this is cpu intensive, optimizations might matter. I remember back in 2011 that I did most of my measurements with a "Release" build for example. But the above tests I did were with a debug build, so ... dunno. Also: one of the last things that Morten Kloster committed in 2011 was another performance improvement, namely "restarting the LCS" in some specific cases. At the time, I didn't have enough time to review this properly, and had some reservations (but mainly lack of time), so I asked him to commit it on a branch (diff-improvements). This feature branch never made it to trunk. Just as another "shot", perhaps you could try r1140247 and see if it helps? -- Johan
Re: [PATCH] limit diff effort to fix performance issue
On Sun, Jun 6, 2021 at 4:57 PM Stefan Sperling wrote:
>
> On Sat, Jun 05, 2021 at 08:56:24PM +0200, Johan Corveleyn wrote:
> > Hmm, I tried this patch with my "annotate large XML file with deep
> > history test", but the result isn't the same as with 1.14. I'd have to
> > investigate a bit more to find out where it took another turn, and
> > what that diff looks like (and whether or not the "other diff" is a
> > problem for me).
>
> It would be good to know how many iterations your file needs to produce
> a diff without a limit. You can use something like SVN_DBG(("p=%d\n", p))
> to print this number.
Okay, I focused on the first revision causing the annotate to differ,
and with some debug logging:
- p went up to 139
- length[0]=1942, length[1]=1817
Now, 1942 lines on the left and 1817 on the right doesn't seem all
that many (that's what remains after prefix-suffix stripping). I see
no reason 'svn diff' shouldn't be able to calculate a minimal diff for
those sizes in a reasonable amount of time. So if p can go up to such
a "cost" for such "reasonable" lenghts, I imagine we should put the
actual limit much higher. I suppose we can just as well set "min_cost"
to 1024 or even higher. 64 is way too low.
BTW, the actual revision (and proper diff) here was not really
unreadable. It had 86 hunks. The log message was something like
"refactor contact parameters", and the change was removing 4 lines and
replacing them with 1 line, throughout an entire section. If the
lcs-limit gets hit, the entire "remaining part" (which normally would
have been some more small hunks) becomes one giant remove+add of 1000
lines or so (i.e. quite useless for a diff that a human might want to
review).
To give you some more numbers about this large XML file:
- it has 17701 revisions
- it has almost 140,000 lines in the HEAD revision
- it's a little less than 4 MB
Annotating the full history (i.e. performing 17700 diffs) takes around
10 minutes, with or without your lcs-limit patch. So it's not like any
of those 17700 diffs are taking a long time. I just tried running it
with min_cost set to 160, and I still get differences. Next I'll try
with 1024.
Finally, a couple of small remarks related to the patch itself:
[[[
+ int max_cost;
+ /* The minimum cost 'p' (# moves away from k=0) we are willing to invest. */
+ const int min_cost = 64;
]]]
I get confused by the naming of the variables here (unless we use
min_max_cost or something). Perhaps something like 'cost_limit' and
'min_cost_limit' or something like that?
[[[
+ max_cost = shift_sqrt(length[0] + length[1] / 2);
]]]
Is that correct WRT operator precedence? It looks like you want to
take the sqrt of the average length, which would be (length[0] +
length[1]) / 2. Or maybe I misunderstand. I'm not really sure why this
is used as an estimate for the "acceptable cost" / max_cost. But then
again, it's been a while since I read the details of Myers algorithm.
--
Johan
Re: [PATCH] limit diff effort to fix performance issue
On Thu, Jun 3, 2021 at 12:57 PM Stefan Sperling wrote:
>
> On Wed, Jun 02, 2021 at 02:20:12PM +0200, Stefan Sperling wrote:
> > The patch below implements an effort limit for Subversion's LCS diff.
>
> Here is an updated version of this patch. I took a closer look at
> other diff implementations again, and realized that they don't use
> a fixed limit but one which is scaled to the size of the Myers graph.
> With a square root applied to keep the effort within certain bounds
> as the files being compared become larger.
>
> Compared to the previous patch, I have added comments, and renamed
> 'max_effort' to 'max_cost' to better align with existing documentation
> of the algorithm in existing comments.
>
> We already maintain a variable 'p' which represents the cost of
> the algorithm. So instead of maintaining a separate counter I am
> now setting an upper bound on 'p'.
>
> With this new patch the limit ends up being 2048 for the files I have.
> For regular files in a working copy of SVN trunk, the loop typically
> needs less than 10 iterations.
>
> For reference, run-times of 'svn diff' on my test files with various
> limits, including no limit:
>
> limit = 1024
> 0m06.82s real 0m05.40s user 0m01.41s system
> limit = 2048
> 0m08.15s real 0m06.55s user 0m01.56s system
> limit = 4096
> 0m11.10s real 0m09.59s user 0m01.52s system
> limit = 8192
> 0m18.40s real 0m16.57s user 0m01.57s system
> limit = 65536
> 7m55.40s real 7m53.81s user 0m01.58s system
> Full run without limit:
> 163m31.49s real 163m16.77s user 0m02.37s system
>
> Git/libxdiff somehow manages to produce a smaller collection of larger
> chunks on the problematic files, rather than many small chunks with
> one giant chunk at the end which we end up produing when we bail out
> of the loop.
> I have made some attempts to produce a more readable diff result in
> case the limit is reached but I haven't been successful. Ideas welcome.
> Still, I don't think it makes sense to invest a lot of time into
> optimizing output in this case, under the assumption that nobody is
> going to be carefully reading affected diffs anyway.
>
> [[[
> * subversion/libsvn_diff/lcs.c
> (shift_sqrt): New helper function. Borrowed from Game of Trees' diff.
> (svn_diff__lcs): Limit the effort spent on finding an optimal diff to
>avoid spinning on the CPU for a long time (30 minutes or more) for
>some input files. Large XML documents were known to trigger this.
> ]]
>
> Index: subversion/libsvn_diff/lcs.c
> ===
> --- subversion/libsvn_diff/lcs.c(revision 1890390)
> +++ subversion/libsvn_diff/lcs.c(working copy)
> @@ -227,7 +227,18 @@ prepend_lcs(svn_diff__lcs_t *lcs, apr_off_t lines,
>return new_lcs;
> }
>
> +/* Integer square root approximation */
> +static int
> +shift_sqrt(apr_off_t val)
> +{
> + int i;
>
> + for (i = 1; val > 0; val >>= 2)
> +i <<= 1;
> +
> + return i;
> +}
> +
> svn_diff__lcs_t *
> svn_diff__lcs(svn_diff__position_t *position_list1, /* pointer to tail
> (ring) */
>svn_diff__position_t *position_list2, /* pointer to tail
> (ring) */
> @@ -247,6 +258,9 @@ svn_diff__lcs(svn_diff__position_t *position_list1
>apr_off_t k;
>apr_off_t p = 0;
>svn_diff__lcs_t *lcs, *lcs_freelist = NULL;
> + int max_cost;
> + /* The minimum cost 'p' (# moves away from k=0) we are willing to invest.
> */
> + const int min_cost = 64;
>
>svn_diff__position_t sentinel_position[2];
>
> @@ -337,6 +351,24 @@ svn_diff__lcs(svn_diff__position_t *position_list1
>fp[d - 1].position[0] = sentinel_position[0].next;
>fp[d - 1].position[1] = _position[1];
>
> + /* Limit the effort spent looking for a diff snake. If files have
> + * very few lines in common, the effort spent to find nice diff
> + * snakes is just not worth it, the diff result will still be
> + * essentially minus everything on the left, plus everything on
> + * the right, with a few useless matches here and there.
> + *
> + * Very large files which have a lot of common lines interleaved with
> + * changed lines (such as humongous auto-generated XML documents) could
> + * easily keep us busy here for a very long time, in the order of hours.
> + * In such cases we abort the algorithm before it is finished and use
> + * the most recently computed intermediate result. The diff will not be
> + * pretty but a quick suboptimal result is better than a perfect result
> + * which takes hours to compute.
> + */
> + max_cost = shift_sqrt(length[0] + length[1] / 2);
> + if (max_cost < min_cost)
> +max_cost = min_cost;
> +
>p = 0;
>do
> {
> @@ -353,7 +385,7 @@ svn_diff__lcs(svn_diff__position_t *position_list1
>
>p++;
> }
> - while (fp[0].position[1] != _position[1]);
> + while (fp[0].position[1] != _position[1] && p < max_cost);
>
>if (suffix_lines)
>
Re: [PATCH] limit diff effort to fix performance issue
On Wed, Jun 2, 2021 at 4:05 PM Daniel Shahaf wrote: > > Stefan Sperling wrote on Wed, 02 Jun 2021 12:20 +00:00: > > The test suite is passing, which implies that trivial diffs aren't affected > > by this change. I expect that most, if not all, diffs which people actually > > want to read will remain unaffected. But I cannot tell with 100% certainty. > > Not with 100% certainty, but you could run «svn diff -c $i ^/» on the > last few thousand revisions of /repos/asf, diff the resulting diffs, and > review the non-empty interdiffs. Another thing I could try is running my old "blame huge XML file with lots of revisions", which I used to do when I worked on diff optimisations back in 2011 (prefix/suffix-scanning and some other things -- also some other people back then did some other optimisation contributions which I double checked this way). I'm guessing my huge XML file is not huge enough (or our diffs not pathological enough) for this limit to have much impact, because after the 2011 optimisations I can't remember running into longer times than say 5 seconds or so per diff. Or otherwise, I can at least give some feedback about a good "effort limit" with a real-world case where we still consider the actual diff output somewhat important (for assigning blame :-)). I'll try to give it a go in a couple of days (but I'll have to dust off my local build first). BTW: for whoever is interested, some discussion about this on IRC just now: https://colabti.org/irclogger/irclogger_log/svn-dev?date=2021-06-02 Feel free to join on #svn-dev on irc.libera.chat ... -- Johan
Re: Migrate off Freenode IRC?
On Thu, May 20, 2021 at 4:24 PM Nathan Hartman wrote: > > On Thu, May 20, 2021 at 7:18 AM Stefan Sperling wrote: >> >> >> In order to signal our move to the new IRC service, I believe we should >> mark our freenode channels as 'moved to libera.chat' in their topic lines, >> as soon as we have consensus in this project that we indeed want to move. >> >> Are there any objections to this? > > > > No objection from me. > > Since channels exist on both services now, I think the freenode channels > should be marked "deprecated" and kept around for some period of time; I have > no particular suggestion on how long that should be, but I guess until the > dust settles and we become confident that libera.chat is stable. > +1 I'm a bit slow picking up things these days (or maybe I've always been slow :-)), but from what I can gather following this from a distance: sure, migration to libera.chat sounds good (plus some form of deprecation on freenode), and future avenues remain to be explored. I'll get there on libera.chat someday in the future :-), but I'm happy to see some of us have already made it there, and taken control of our channels over there. -- Johan
Re: Commit reviews' author statistics: bus factor issue?
On Sun, Apr 25, 2021 at 4:44 PM Nathan Hartman wrote: > > On Fri, Apr 23, 2021 at 9:40 AM Daniel Shahaf wrote: > > > > Nathan Hartman wrote on Thu, 22 Apr 2021 21:41 +00:00: > > > Not knowing whether / how many people have reviewed a particular > > > commit is, as was said elsewhere, a silent failure mode of the CTR > > > (commit-then-review) convention. > > > > > > Do we want to try switching to a RTC (review-then-commit) convention? > > > > If we do switch to RTC, we might want to also retroactively ensure all > > commits post 1.14.x's branching have been reviewed by at least two pairs > > of eyes each. > > > > However, I wonder whether there's a smaller change we can do first, > > rather than a full-blown s/CTR/RTC/ flag day. For instance, how > > about we agree, for the next N weeks, to make our commit reviews > > explicit? I.e., to explicitly say "I've reviewed this commit and > > found no issues"? (Call this Commit-Then-Explicit-Review.) > > I'm +1 to this idea. It sounds to me like a reasonable middle ground > to gain some insights before making bigger workflow-altering changes. I'm doubtful that this will improve our bus factor for reviews, except perhaps by drawing in some more reviewer-attention by seeing more of it on the list. So far I'm mostly seeing reviews by DanielSh and Nathan. Switching from CTR to CTER (or to RTC) will not really change that. It might change the _potential impact_ of DanielSh being hit by a bus (mainly that Nathan's commits will get stuck in "no explicit review by someone else" (CTER) vs. "on trunk but reviewed by no one else" (CTR)). Okay, with CTR this is less visible at first, and might bite us much harder later on, but we'll have a big problem in any case if one of our few remaining volunteers loses the ability / energy / time / interest to do their bit. IMHO the only thing that can really help is attracting more active contributors who are willing and able to spend some of their precious time on the development of Subversion. -- Johan
Re: Commit reviews' author statistics: bus factor issue?
On Thu, Apr 22, 2021 at 9:22 PM Daniel Shahaf wrote:
>
> [ Forwarding from private@ with an addition between triple dashes and
> some paragraphs omitted altogether. ]
>
> Methodology: In my dev@ mailbox, I looked at "Re: svn commit" threads
> where the subject line contained "trunk" somewhere, filtered by date
> (using, e.g., «~s 'Re: svn commit' !~<( ~s 'Re: svn commit' ) ~d '<730d'
> ~s trunk» in Mutt¹). I then did a author histogram (the moral equivalent
> of «SELECT author, COUNT(*) AS cnt FROM results_of_the_filter GROUP BY
> author ORDER BY cnt»).
>
> With the date filter set to ">6 years ago", the histogram is:
> .
> 1, 1, 1, 1, 2, 3, 6, 7, 10, 12, 13, 13, 19, 27, 49, 58, 86
> .
> Top three: 28.1%, 19.0%, 16.0%.
>
> With the date filter set to "<2 years ago", the histogram is:
> .
> 1, 1, 1, 1, 1, 1, 1, 1, 4, 5, 30
> .
> Top three: 64%, 10.6%, 8.5%.
>
> Do we have a bus factor problem?
>
> ---
>
> I'm deliberately not posting the author identities part of the
> histograms. It's public info (and I literally did just post
> instructions for how to compute it, for reproducibility), but
> no individual's contributions or contribution statistics are the point.
>
> The histogram is of the authors of commit review threads, not of
> everyone who participated in such threads.
>
> ---
>
> Having few reviewers is problematic in various ways:
>
> - Bus factor
>
> - Single point of failure (cf. Linus' Law)
>
> - Possibility of zero reviews for some areas of the code
>
> - Review standards should be seen as community standards rather than
> a reviewer's idiosyncrasies; cf. the point about new projects needing
> at least two mentors ("parents"), rather than just one
>
> - [not an exhaustive list]
>
> Cheers,
>
> Daniel
>
> ¹ There may be a better way to express "first in a thread". I tried
> «!~<(^)», but couldn't get it to work.
Good point. But I believe we have many other areas where our bus
factor is getting very low.
- RM -- after Julian, only Stefan volunteered. We seem to depend a lot
on the available volunteer time of just a single person, for whether
or not a release will be made and in what timeframe.
- Security issues: sometimes they linger for a long time because ...
well they just lie there. Sometimes some of us perform a couple of
steps, but then we stall again waiting for reviewers, other people to
chime in, etc. This can take weeks or months of "standstill", and I'm
guessing some of the people pushing the cart often feel "I'll give a
little push because it's been stuck for too long, and it's getting
embarrasing -- otherwise it might be lying here for years"
- Approving backports. Last time we really had to scrape the very few
resources we have left to get anything backported (I used to almost
never spend any effort on reviewing backports -- there were plenty of
other more experienced devs around -- but this time I more or less
felt "forced" to go that extra mile).
- Signing releases: last couple of years I'm the only one to build /
test / sign for Windows. I have often wondered how long one of our
releases would be stuck if I were to disappear / drop my laptop.
Perhaps the lack of review-activity for us as a CTR project is more
critical, I don't know. Good observation in any case.
--
Johan
Re: JavaHL test failure and warning in 1.14.1
On Thu, Feb 18, 2021 at 10:19 PM Alexandr Miloslavskiy wrote: > > Going to take longer, sorry. I'm bombarded with things to take care > of... While trying to have vacation, huh. Again, of the flaky test > stands in the way, I wouldn't mind of you just comment it out for now. Please enjoy your vacation first and foremost, Alexandr :-). I hope you don't get bombarded too much. There is nothing on fire here, and as you say: the test can be commented out or otherwise disabled (as I understand it, the test is simply exercising the code in a particular way, so if anything errors out or crashes, it's not really the test's fault). We should definitely look further into it (and I can try to make some time for this too, next week or so). But it's nothing that can't wait for a couple more days / weeks, IMHO. Cheers, -- Johan
Re: JavaHL test failure and warning in 1.14.1
On Fri, Feb 12, 2021 at 6:36 PM James McCoy wrote: > > One of the new JavaHL tests > (testCrash_RequestChannel_nativeRead_AfterException) is failing on > Debian's armhf, mips64el, mipsel, and powerpc builders: > > https://buildd.debian.org/status/logs.php?pkg=subversion=1.14.1-1=sid > > There was 1 failure: > 1) > testCrash_RequestChannel_nativeRead_AfterException(org.apache.subversion.javahl.BasicTests)junit.framework.AssertionFailedError: > IOException was caught in run() > at > org.apache.subversion.javahl.BasicTests$TestTunnelAgent.joinAndTest(BasicTests.java:4477) > at > org.apache.subversion.javahl.BasicTests.testCrash_RequestChannel_nativeRead_AfterException(BasicTests.java:4679) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method) > at > java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) > at > java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) > at org.apache.subversion.javahl.RunTests.main(RunTests.java:119) > > FAILURES!!! > Tests run: 147, Failures: 1, Errors: 0 > > On most of those, we're also getting these warnings: > > OpenJDK 64-Bit Zero VM warning: You have loaded library > subversion-1.14.1/BUILD/subversion/bindings/javahl/native/.libs/libsvnjavahl-1.so.0.0.0 > which might have disabled stack guard. The VM will try to fix the stack > guard now. > It's highly recommended that you fix the library with 'execstack -c > ', or link it with '-z noexecstack'. > .WARNING in native method: JNI call made without > checking exceptions when required to from CallVoidMethodV > at java.lang.Object.getClass(java.base@11.0.10/Native Method) > at > java.util.ArrayList.equals(java.base@11.0.10/ArrayList.java:560) > at > org.apache.subversion.javahl.BasicTests.testBasicChangelist(BasicTests.java:2626) > at > jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@11.0.10/Native > Method) > at > jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@11.0.10/NativeMethodAccessorImpl.java:62) > at > jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@11.0.10/DelegatingMethodAccessorImpl.java:43) > at > java.lang.reflect.Method.invoke(java.base@11.0.10/Method.java:566) > at junit.framework.TestCase.runTest(TestCase.java:177) > at junit.framework.TestCase.runBare(TestCase.java:142) > at junit.framework.TestResult$1.protect(TestResult.java:122) > at junit.framework.TestResult.runProtected(TestResult.java:142) > at junit.framework.TestResult.run(TestResult.java:125) > at junit.framework.TestCase.run(TestCase.java:130) > at junit.framework.TestSuite.runTest(TestSuite.java:241) > at junit.framework.TestSuite.run(TestSuite.java:236) > at junit.framework.TestSuite.runTest(TestSuite.java:241) > at junit.framework.TestSuite.run(TestSuite.java:236) > at junit.textui.TestRunner.doRun(TestRunner.java:116) > at junit.textui.TestRunner.doRun(TestRunner.java:109) > at junit.textui.TestRunner.run(TestRunner.java:77) > at org.apache.subversion.javahl.RunTests.main(RunTests.java:119) > > If I re-run the tests without clearing out > /subversion/bindings/javahl/test-work, then the test passes. > Hopefully that helps provide some insight. > > I can run tests as needed on Debian's porterboxes. [ cc += Alexandr Miloslavkiy, as he wrote those new tests ] Thanks for reporting these, James. Which JDK version is this? Or is it different on the different architectures? Did you also test 1.14.0 previously with the same JDK, and you didn't see those warnings then? -- Johan
Re: svn commit: r45955 - /dev/subversion/ /release/subversion/
On Wed, Feb 10, 2021 at 2:23 PM Stefan Sperling wrote: > > On Wed, Feb 10, 2021 at 02:16:31PM +0100, Johan Corveleyn wrote: > > On Wed, Feb 10, 2021 at 12:27 PM wrote: > > > > > > Author: stsp > > > Date: Wed Feb 10 11:27:48 2021 > > > New Revision: 45955 > > > > > > Log: > > > Publish Subversion-1.14.1. > > > > Hi Stefan, > > > > Don't we need at least 3 votes? Or did you mean to count also your own > > (implicit perhaps, as RM), in addition to Brane's and mine? If so, I'm > > inclined to think that you should also explicitly state your "+1 for > > release" on the dev@ vote thread (including your test details, > > platform, etc ... as usual). Otherwise it would be a bit unclear > > (especially since we rarely counted the RM's vote as an extra release > > vote, certainly not implicitly). > > Yes, I counted my own vote towards the total of 3. > We need 3 PMC members to bless a release. I can add my votes to the thread. +1 > I did not want to sit on the security fix any longer. > The embargo elapsed today and distributions are expecting a fix to show up. Ack, thanks. -- Johan
Re: Subversion 1.10.7 up for testing/signing
On Thu, Feb 4, 2021 at 1:57 PM Stefan Sperling wrote: > > The 1.10.7 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. > > Thanks! Summary --- +1 to release Platform Windows 10 x64 (Version 1903) Microsoft Visual Studio 2019 Community Edition (Version 16.5.3) Verified Signature, sha1 and sha512 for subversion-1.10.7.zip. Contents of subversion-1.10.7.zip are identical to tags/1.10.7, and to branches/1.10.x@1886195 (except for expected differences in svn_version.h and svnpubsub, svnwcsub and nominate.pl (symlinks vs. file contents), and generated files). Tested -- [ Release build x86 ] x [ fsfs ] x [ file | svn | http ] Results --- All tests pass. Dependencies httpd 2.4.43 (apr 1.6.5, apr-util 1.6.1 pcre 8.44, expat 2.2.9, openssl 1.1.1f) apr 1.6.5 apr-util 1.6.1 openssl 1.1.1f serf 1.3.9 sqlite 3.31.1.0 zlib 1.2.11 (bundled lz4 1.7.5) (bundled utf8proc 2.1.0) Other tools --- perl 5.20 (Strawberry Perl) python 2.7.17 Signature - subversion-1.10.7.zip: -BEGIN PGP SIGNATURE- iQIcBAABCgAGBQJgIDMeAAoJELWc5tYBDIqtXWAP/0fYa6wGyVbvcpcnPXKM760D aEBjTqs3/YbFN3ZeRaOJ4ewDQ7zWYXH0FGCoxuZEBSIkfXUT4FAnyKJrtfhuY9pD SZkXzssB7rsgrWmSBBiZ8CldMFsXdRmH/CwnV/ii9fykGNOa9C1DqgsfAxCJeIMu 8yzVzEZvRnbpBzEL/HirJJ7JmdffW6/Wb8vq+8+NVjlInEZuy+JDOUOQ4CW6v7sG gE+mVPhWWIxYHgqRrluNYq9LXS/NYZsCczUsq/lWZXbz+9zVXJO5ozgPxAJY+ZIz zr8QE3HryJaK/pBbwJVRXmO31m0vGLpelj635h9kl5ZUo8SCW2PTcj0gQca8zuXa MmE1Ropn/HrOKE927jwXAlZJQ89xKNLR7JR/x3EXbTjmeclqqjp7638XBLEe7J9s NKS/n419rRSmLAQfBYPa3cSvecpRglVkDdFWLogFeYpxdwsEqLFJEWAjZeysF55z IPyOxvT8/y+gAl1w5ai6emX0YW63ngnUUr7ZnOHVXhaKiLb3tLoafOePRPGoBED4 0YBRTmONboBeXayQ//O+QMsiQmhZpsE6G9+nr/GmYRR4G+A7az6FwGtKoBd80zSc j3vBQbbXWPsWI07eqVIrD6ARv36YjoTkR5Cuqh09+yi/Gj2soanHH2zXIvt1E64i +2eAxVY76YZiwJTYUlL9 =6LYN -END PGP SIGNATURE- -- Johan
Re: Subversion 1.14.1 up for testing/signing
On Thu, Feb 4, 2021 at 1:56 PM Stefan Sperling wrote: > > The 1.14.1 release artifacts are now available for testing/signing. > Please get the tarballs from > https://dist.apache.org/repos/dist/dev/subversion > and add your signatures there. > > Thanks! Summary --- +1 to release (Windows) Platform Windows 10 x64 (Version 1903) Microsoft Visual Studio 2019 Community Edition (Version 16.5.3) Verified Signature and sha512 for subversion-1.14.1.zip. Contents of subversion-1.14.1.zip are identical to tags/1.14.1, and to branches/1.14.x@1886195 (except for expected differences in svn_version.h and svnpubsub, svnwcsub and nominate.pl (symlinks vs. file contents), and generated files). Tested -- [ Release build x64 ] x [ fsfs ] x [ file | svn | http ] javahl swig-python Results --- All tests pass. Dependencies httpd 2.4.43 (apr 1.6.5, apr-util 1.6.1, pcre 8.44, expat 2.2.9, openssl 1.1.1g) apr 1.6.5 apr-util 1.6.1 openssl 1.1.1g serf 1.3.9 sqlite 3.31.1.0 zlib 1.2.11 python 3.9.1 py3c 1.0 swig 4.0.1 (bundled lz4 1.7.5) (bundled utf8proc 2.1.0) Other tools --- perl 5.30.2 (Strawberry Perl) python 3.9.1 AdoptOpenJDK 11.0.6 junit 4.12 Signature - subversion-1.14.1.zip: -BEGIN PGP SIGNATURE- iQIcBAABCgAGBQJgICJ8AAoJELWc5tYBDIqtgqkP/2xOi+s+iose90xD4wtdJgVq xwJ3gcCxws1HHdcMNtUudP740gI7J4BFNEDExuQLNQ8RjhysvXTKEO7nhwo4GyX9 HgKwafyyrRy6dTQ6tBAl0FC40GeACa2CSXMJfmn2I0UBKUe7RgG4l806FoI/Amw0 i/IQjwcqLYdNhBfkv70XoJxW35W1nVE9KsELVN7KJk1JRXsJRZ0+VyUjE5pBulYH 7FYoNNOgl1O9eJKRjSQXGD+FltYhYSi84uJ4fXntOZGEztqvbATmSw52wrKyZpOX ewzjltvknbgNXbYcs8Xm/0WVaB0QH43gqZfUZJ4wtZcTrqtf6bgtEXoVk7HIpRZG Jw/tqcmNIuGXTchZRu2eTNW2O9LdCv68r1NnMFhk6sFvwiyKmtOslCUItiAaYx0b Yj4MYeG39it9+I6I3eM+9zeJqGiJYR+vhKCDf8G0GiwA2oJktBqcW3xHlx814IiE /7AtaMz9Uw2rAgaHGJbyxTtskddK6rCpEX4UX3LXk8v4C44UYcNEpBeZvVCp7Ava VWAaiofdqeaUneDxNQ9Ji4JoojT2hhNvQuXjKxI+JZTqIawCvxiyW837hK/QlZnT A4zZ1NWMkBTxHIXfcJ4tqH/57LgB3YQXELrMcXAZ8w8oc7iuW2fN2XAEBqKRtGk8 2Zf01GEAIl1J8WD3UOS/ =6k// -END PGP SIGNATURE- -- Johan
Re: Release note entry for 1.14.1 Fix for issue #4762 "authz doesn't combine global and repository rules"?
On Fri, Jan 29, 2021 at 9:46 PM Stefan Sperling wrote: > > On Fri, Jan 29, 2021 at 09:29:52PM +0100, Johan Corveleyn wrote: > > I was wondering whether the change in authz behavior, by fixing #4762, > > should be called out explicitly in the 1.14 release notes (maybe > > somewhere in the "Known issues" section, describing the issue of > > 1.14.0, and how it is fixed in 1.14.1). > > Yes. Thanks for the reminder. I will try to keep this in mind > and update the web site when the fixes are released. > > > And if we would also backport it to 1.10.x (it currently has 2 votes > > there), of course the same there. > > It is already listed as a known problem in 1.10: > http://subversion.apache.org/docs/release-notes/1.10.html#issue-svn-4762 Ah yes, thanks. I missed that. -- Johan
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
On Fri, Jan 29, 2021 at 9:29 PM Alexandr Miloslavskiy wrote: > > On 29.01.2021 20:51, Johan Corveleyn wrote: > > > Fingers crossed for a second vote in STATUS now :-). > > Did that, hopefully I didn't mess anything up, it's my first experience > with it :) Looks good! Looks like everything is good to go now for your javahl fixes in 1.14.1 :-). Just to clarify: backports to bindings require only 2 votes (in fact even only one +1, and one +0 or "concept +1"). Backports to core code require 3 votes (or for non-LTS releases also just 2 votes). See [1] for the official explanation. And, as you did correctly, usually the one who adds the last needed vote moves the section below the "Approved changes" line (provided there are no vetoes). This helps for our nightly backport bot which automatically processes the STATUS file, and merges everything below the Approved line (with the svn-role account). [1] http://subversion.apache.org/docs/community-guide/releasing.html#release-stabilization-how-many-votes -- Johan
Release note entry for 1.14.1 Fix for issue #4762 "authz doesn't combine global and repository rules"?
I was wondering whether the change in authz behavior, by fixing #4762, should be called out explicitly in the 1.14 release notes (maybe somewhere in the "Known issues" section, describing the issue of 1.14.0, and how it is fixed in 1.14.1). And if we would also backport it to 1.10.x (it currently has 2 votes there), of course the same there. Some admins might be surprised by the change and impact on their existing authz files, and might appreciate the discoverability via the release notes. (I'm not volunteering, sorry (don't understand it enough, and I'm a bit out of svn cycles). Just putting it out here.) -- Johan
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
On Thu, Jan 28, 2021 at 11:28 AM Johan Corveleyn wrote: ... > With that, the branch looks good to me, and I think we should merge > this to trunk, and then nominate it (the merge-to-trunk commit I > guess) for backport to 1.14 (concretely, this means adding an entry to > 1.14.x/STATUS [1]). > > @Alexandr: would you like to do the honors? First to merge your branch > to trunk, and after that to add a corresponding section to > 1.14.x/STATUS? > > In fact: please consider your partial committership (javahl bindings) > now to be no longer confined to branches. Feel free to perform commits > to the javahl bindings area also on trunk (which includes the above > merge of course). > This also means you have a binding vote on backport proposals related > to the javahl bindings. > > [1] https://svn.apache.org/repos/asf/subversion/branches/1.14.x/STATUS Small update for whoever is following along: I just merged the branch to trunk in r1886029, and nominated it in 1.14.x/STATUS. To assure the list that I'm not bypassing Alexandr here: we mailed offlist about this, and he preferred that I would perform the merge, so I did :-). Fingers crossed for a second vote in STATUS now :-). -- Johan
Re: Escaping SVN_EDITOR broken on Windows
On Thu, Jan 28, 2021 at 4:22 PM Yasuhito FUTATSUKI
wrote:
>
> On 2021/01/28 20:13, Johan Corveleyn wrote:
> > On Thu, Jan 28, 2021 at 12:55 AM Yasuhito FUTATSUKI
> > wrote:
> >>
> >> On 2021/01/28 7:33, Johan Corveleyn wrote:
> >>> On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
> >>> wrote:
> >>>>
> >>>> On 2021/01/28 5:43, Johan Corveleyn wrote:
> >>>>> On Mon, Mar 16, 2020 at 5:11 AM wrote:
> >>
> >>
> >>>>> [[[
> >>>>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession
> >>>>> -multiInst
> >>>>>
> >>>>> C:\test>svn pe svn:ignore .
> >>>>> 'C:\Program' is not recognized as an internal or external command,
> >>>>> operable program or batch file.
> >>>>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> >>>>> -nosession -multiInst "svn-prop.tmp"') returned 1
> >>>>> ]]]
> >>>>
> >>>> Perhaps my pending patch also fix this.
> >>>>
> >>>> Please see
> >>>>
> >>>> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
> >>>>
> >>>> and attached patch
> >>>>
> >>>> https://lists.apache.org/api/email.lua?attachment=true=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
> >>>
> >>> Yes indeed, that seems to fix it :). Nice!
> >>>
> >>> Is anything holding this patch back from being committed? We're mainly
> >>> a CTR - Commit Then Review - project, so if you feel okay about it ...
> >>> I haven't studied it in depth, but on a cursory look, it seems fine to
> >>> me (and it works / fixes my problem, yay!).
> >> Then I commited it in r1885953, with minor fix in comment.
> >
> > Great!
> >
> > I took the liberty to nominate it (with my vote) for 1.14.x, not in
> > the least because I'm personally affected by this (always a good
> > motivation :-)). Not sure if it'll gather enough votes in time to make
> > it into 1.14.1, but it doesn't hurt to try :-).
>
> Unfortunately this causes conflict on 1.14.x because it depends on r1882234.
Argh, sorry should have noticed that. Hmmm.
Should I withdraw the nomination? Or should I add r1882234 to it? I
don't know the context of these revisions very well ...
--
Johan
Re: Escaping SVN_EDITOR broken on Windows
On Thu, Jan 28, 2021 at 12:55 AM Yasuhito FUTATSUKI
wrote:
>
> On 2021/01/28 7:33, Johan Corveleyn wrote:
> > On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
> > wrote:
> >>
> >> On 2021/01/28 5:43, Johan Corveleyn wrote:
> >>> On Mon, Mar 16, 2020 at 5:11 AM wrote:
>
>
> >>> [[[
> >>> C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession
> >>> -multiInst
> >>>
> >>> C:\test>svn pe svn:ignore .
> >>> 'C:\Program' is not recognized as an internal or external command,
> >>> operable program or batch file.
> >>> svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> >>> -nosession -multiInst "svn-prop.tmp"') returned 1
> >>> ]]]
> >>
> >> Perhaps my pending patch also fix this.
> >>
> >> Please see
> >>
> >> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
> >>
> >> and attached patch
> >>
> >> https://lists.apache.org/api/email.lua?attachment=true=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
> >
> > Yes indeed, that seems to fix it :). Nice!
> >
> > Is anything holding this patch back from being committed? We're mainly
> > a CTR - Commit Then Review - project, so if you feel okay about it ...
> > I haven't studied it in depth, but on a cursory look, it seems fine to
> > me (and it works / fixes my problem, yay!).
> Then I commited it in r1885953, with minor fix in comment.
Great!
I took the liberty to nominate it (with my vote) for 1.14.x, not in
the least because I'm personally affected by this (always a good
motivation :-)). Not sure if it'll gather enough votes in time to make
it into 1.14.1, but it doesn't hurt to try :-).
Cheers,
--
Johan
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
On Thu, Jan 28, 2021 at 1:15 AM Alexandr Miloslavskiy wrote: > > On 27.01.2021 13:00, Johan Corveleyn wrote: > > > ^^ some lines indented with tabs instead of spaces > > > ^^ curly brace should be on next line > > > ^^ The comment above, describing how the JVM crashes, refers to the > > situation before you actually fixed that :-). Can you rephrase it a > > bit, so it is still applicable even with the crash now fixed? > > > ^^ same here, the comments describe the behavior before the fix, maybe > > rephrase it so it's still valid after you fixed it. > > Thanks, I fixed everything in r1885955. Thanks. I see you did a bit more than what I mentioned above (I guess you did a "reformat" of the entire section you added), but that's fine of course (I only spotted a couple of tidbits, not everything :)). With that, the branch looks good to me, and I think we should merge this to trunk, and then nominate it (the merge-to-trunk commit I guess) for backport to 1.14 (concretely, this means adding an entry to 1.14.x/STATUS [1]). @Alexandr: would you like to do the honors? First to merge your branch to trunk, and after that to add a corresponding section to 1.14.x/STATUS? In fact: please consider your partial committership (javahl bindings) now to be no longer confined to branches. Feel free to perform commits to the javahl bindings area also on trunk (which includes the above merge of course). This also means you have a binding vote on backport proposals related to the javahl bindings. [1] https://svn.apache.org/repos/asf/subversion/branches/1.14.x/STATUS -- Johan
Re: Escaping SVN_EDITOR broken on Windows
On Wed, Jan 27, 2021 at 10:19 PM Yasuhito FUTATSUKI
wrote:
>
> On 2021/01/28 5:43, Johan Corveleyn wrote:
> > On Mon, Mar 16, 2020 at 5:11 AM wrote:
> >>
> >> Author: jamessan
> >> Date: Mon Mar 16 04:11:36 2020
> >> New Revision: 1875230
> >>
> >> URL: http://svn.apache.org/viewvc?rev=1875230=rev
> >> Log:
> >> Followup to r1874093, add Windows-specific argument escaping
> >>
> >> Rather than directly using apr_pescape_shell(), use apr_escape_shell() to
> >> give
> >> an indication whether escaping is needed. If APR reports no escaping is
> >> needed, simply surround the argument in double-quotes to handle any
> >> embedded
> >> whitespace.
> >>
> >> When escaping is needed, on Unix we continue to use APR's escaping +
> >> post-processing for whitespace. On Windows, perform the escaping
> >> ourselves per
> >> these rules:
> >>
> >> 1. Surround the string with double-quotes
> >> 2. Escape any double-quotes or backslashes preceding a double-quote
> >> 3. Escape any metacharacters, including double-quotes, with ^
> >>
> >> * subversion/libsvn_subr/cmdline.c
> >> (escape_path): Refactored as above
> >
> > I'm sorry I didn't notice this before, but on Windows, since this
> > commit r1875230 (which is included in 1.14.0) the escaping of
> > SVN_EDITOR is broken. If the envvar refers to a program with a space
> > in its path, between quotes, those quotes seem to be lost now, which
> > results in:
> >
> > [[[
> > C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession
> > -multiInst
> >
> > C:\test>svn pe svn:ignore .
> > 'C:\Program' is not recognized as an internal or external command,
> > operable program or batch file.
> > svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
> > -nosession -multiInst "svn-prop.tmp"') returned 1
> > ]]]
>
> Perhaps my pending patch also fix this.
>
> Please see
>
> https://lists.apache.org/thread.html/r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154%40%3Cdev.subversion.apache.org%3E
>
> and attached patch
>
> https://lists.apache.org/api/email.lua?attachment=true=r2b8b0b4ca0f833e371590fadb33ee24c6335127e967664c6e612e154@%3Cdev.subversion.apache.org%3E=d76c6fedb56414d73e0d481910f7e62bde6d0582e975c4b2134c8521b5f9e30f
Yes indeed, that seems to fix it :). Nice!
Is anything holding this patch back from being committed? We're mainly
a CTR - Commit Then Review - project, so if you feel okay about it ...
I haven't studied it in depth, but on a cursory look, it seems fine to
me (and it works / fixes my problem, yay!).
--
Johan
Re: svn commit: r1882523 - in /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl: native/ src/org/apache/subversion/javahl/util/
On Wed, Jan 27, 2021 at 10:47 PM Alexandr Miloslavskiy wrote: > > On 27.01.2021 11:34, Johan Corveleyn wrote: > >> + jobject jrequest; > >> + jobject jresponse; > > > > These new fields are not mentioned in the log message. > > > >> @@ -523,7 +527,10 @@ jobject create_Channel(const char *class > >> jmethodID ctor = env->GetMethodID(cls, "", "(J)V"); > > > > ^^ also not mentioned in log message. > > > > (rest looks fine, so if you could amend the commit message with the > > missing details, that'd be great) > > I tried to change commit message, not sure if it actually worked. > > Can you please check? Yes, perfectly. Thanks. BTW, if you would subscribe to comm...@subversion.apache.org (by sending a mail to commits-subscribe@s.a.o), you would see the log-message-changes (and all commits) from that side too. Not required of course, but I use it as a means to follow along a bit, and to double verify my own commits / changes :). -- Johan
Re: svn commit: r1882522 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
On Wed, Jan 27, 2021 at 10:32 PM Alexandr Miloslavskiy
wrote:
>
> On 27.01.2021 11:13, Johan Corveleyn wrote:
> >> +void
> >> +OperationContext::closeTunnel(void *tunnel_context, void *)
> >> +{
> >> + TunnelContext* tc = static_cast(tunnel_context);
> >> + jobject jclosecb = tc->jclosecb;
> >> + delete tc;
> >> +
> >> + JNIEnv *env = JNIUtil::getEnv();
> >> + if (JNIUtil::isJavaExceptionThrown())
> >> +return;
> >> +
> >> + if (jclosecb)
> >> +callCloseTunnelCallback(env, jclosecb);
> >> }
> >>
> >>
> >
> > Here above in closeTunnel(), you dropped the early exit on if
> > (!jclosecb) (before the call to JNIUtil::getEnv()). Was this
> > intentional?
> >
> > Previously, in the case of a null jclosecb, the unnecessary calls to
> > JNIUtil::getEnv() and JNIUtil::isJavaExceptionThrown() were also
> > avoided. In the new version they aren't (not sure if that's important,
> > but it's a small behavioral difference).
>
> Yes, this is intentional. In r1882523, I further extend the cleanup by
> also cleaning 'jrequest' and 'jresponse'. In this case, early exit is no
> good, because everything needs to be cleaned independently. For the
> purpose of reducing the amount of diffs, I removed the early exit one
> patch earlier.
>
> I believe that calling 'JNIUtil::getEnv()' and
> 'JNIUtil::isJavaExceptionThrown()' is merely a negligible performance
> change. I thought that it's not worthy to describe in commit message,
> but I could do that if you think otherwise.
Thanks for explaining. Hm, maybe it would be good to note this in some
form in the commit message, yes. Just to make sure others scrolling
through this in the future are not puzzled by it. Something like:
"While here, remove the early exit on null jclosecb for $reasons (also
see the followup commits where cleanup of ... is further improved, and
everything needs to be cleaned up independently)". (this is just a
quick off the cuff example, feel free to word it how you would best
describe it)
Brane also replied (on dev@) this morning, giving a cleanup-argument too:
"It's a minor performance degradation in the case where there's no
callback, but it is in fact safer because it catches more Java
exceptions during tunnel destruction. The null check is still there, so
that's fine. IMO (and it's been a long time) this change is good."
So I'm entirely convinced :-).
--
Johan
Re: svn commit: r1882520 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/JNIUtil.cpp
On Wed, Jan 27, 2021 at 10:16 PM Alexandr Miloslavskiy wrote: > > Thanks for reviewing! You're quite the savior of this branch :) Heh, you're welcome :). And thanks for bringing it here in the first place. > On 27.01.2021 11:02, Johan Corveleyn wrote: > > was wondering what you meant by "This error is easily seen > > when running JavaHL tests" > > If you checkout r1882518, tests die with 'Bad global or local ref passed > to JNI' before this error can be seen. However, if you also cherry-pick > r1882522 on top to fix this problem, that's where you'll see 'JNI call > made with exception pending' that I fixed in r1882520. Ah, okay. I'll give that another try. > Sorry for confusion. I edited/reordered patches a few times before > committing, and didn't notice that the commit message is not too good > with this ordering. > > Which brings me to next question. In other replies, you suggested that I > make some edits in code and also amend commit messages. How do I do that > in SVN? I understand that unlike git, a submitted branch is already set > in stone? Please advise. This branch is my sole SVN experience in entire > life, so I'm quite a newbie. I did google and ask another guy, though, > to no avail. Well, committed content is indeed "set in stone", so to speak (i.e. "part of history"). You could of course start a new branch and redo everything, but I wouldn't recommend it (that would force me / others to review it all again). The usual practice, if there are remarks about the content, is to simply add additional commits fixing the problems / adding more code / ... So for changes to the contents of the file, I suggest you perform further commits on the branch. OTOH, _commit messages_ are editable in SVN. They are part of the so called "revision properties" (contrary to "versioned properties"). So, for remarks about the commit messages, the usual practice is to simply edit them. For example with this command: svn propedit --revprop -r$REV svn:log $URL (the $URL can be any url pointing to the svn repository, since revision numbers are global for the repo) This will open the current log message in your $SVN_EDITOR (fallback to $EDITOR). > > I do get 8 warnings with this sort of message (always similar > > stacktrace, always coming from UtilTests.testFileMerge, line 120): > > [[[ > > WARNING: JNI local refs: 33, exceeds capacity: 32 > > at java.net.NetworkInterface.getAll(java.base@11.0.6/Native Method) > > at > java.net.NetworkInterface.getNetworkInterfaces(java.base@11.0.6/NetworkInterface.java:359) > > To my knowledge, this is an oversight in > 'java.net.NetworkInterface.getAll' itself. > > JNI documentation says [1]: > [[[ > You can call the JNI EnsureLocalCapacity() method to tell the JVM that > you'll be using more than 16 local references. This allows the JVM to > optimize the handling of local references for that native. Failure to > inform the JVM can lead to a FatalError if the required local references > cannot be created, or poor performance that's due to a mismatch between > the local-reference management employed by the JVM and the number of > local references used. > ]]] > > I'm observing the same warnings in other Java programs which are > unrelated to SVN. I think that this can be ignored for the purposes of > this JavaHL related discussion. > > [1] https://developer.ibm.com/languages/java/articles/j-jni/ Yes, let's ignore that here ... it's unrelated. -- Johan
Escaping SVN_EDITOR broken on Windows (was: svn commit: r1875230 - /subversion/trunk/subversion/libsvn_subr/cmdline.c)
On Mon, Mar 16, 2020 at 5:11 AM wrote:
>
> Author: jamessan
> Date: Mon Mar 16 04:11:36 2020
> New Revision: 1875230
>
> URL: http://svn.apache.org/viewvc?rev=1875230=rev
> Log:
> Followup to r1874093, add Windows-specific argument escaping
>
> Rather than directly using apr_pescape_shell(), use apr_escape_shell() to give
> an indication whether escaping is needed. If APR reports no escaping is
> needed, simply surround the argument in double-quotes to handle any embedded
> whitespace.
>
> When escaping is needed, on Unix we continue to use APR's escaping +
> post-processing for whitespace. On Windows, perform the escaping ourselves
> per
> these rules:
>
> 1. Surround the string with double-quotes
> 2. Escape any double-quotes or backslashes preceding a double-quote
> 3. Escape any metacharacters, including double-quotes, with ^
>
> * subversion/libsvn_subr/cmdline.c
> (escape_path): Refactored as above
I'm sorry I didn't notice this before, but on Windows, since this
commit r1875230 (which is included in 1.14.0) the escaping of
SVN_EDITOR is broken. If the envvar refers to a program with a space
in its path, between quotes, those quotes seem to be lost now, which
results in:
[[[
C:\test>set EDITOR="C:\Program Files\Notepad++\notepad++" -nosession -multiInst
C:\test>svn pe svn:ignore .
'C:\Program' is not recognized as an internal or external command,
operable program or batch file.
svn: E200012: system('"C:\Program Files\Notepad++\notepad++"
-nosession -multiInst "svn-prop.tmp"') returned 1
]]]
This used to work (it works in 1.13.x), I've been using this for a long time.
This commit was the last of three commits related to escaping:
r1874057 (Escape filenames when invoking $SVN_EDITOR)
r1874093 (Followup to r1874057, escape whitespace instead of quoting filename)
r1875230 (Followup to r1874093, add Windows-specific argument escaping)
Interestingly:
- before all three: works
- after r1874057: fails
- after r1874093: works
- after r1875230: fails
Apparently, there was a test failing on Windows after r1874093, which
was pointed out by Bert in this thread:
https://lists.apache.org/thread.html/r8b61c011f4ab66006dd96d61d0e099da03da74b6e8b1d6f73cf1baa0%40%3Cdev.subversion.apache.org%3E
Perhaps this should have been fixed in another way than the approach
of r1875230? Or maybe it just needs another small tweak? I haven't
investigated further (I have pretty much used up my 'svn cycles' these
last couple of days), but I wanted to highlight this nonetheless. If
anyone wants me to try something out on Windows, just say the word ...
Links to the revisions on viewvc:
https://svn.apache.org/r1874057
https://svn.apache.org/r1874093
https://svn.apache.org/r1875230
I'm leaving the diff from the last commit mail here below for quick
scanning, in case it helps anyone.
> Modified:
> subversion/trunk/subversion/libsvn_subr/cmdline.c
>
> Modified: subversion/trunk/subversion/libsvn_subr/cmdline.c
> URL:
> http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cmdline.c?rev=1875230=1875229=1875230=diff
> ==
> --- subversion/trunk/subversion/libsvn_subr/cmdline.c (original)
> +++ subversion/trunk/subversion/libsvn_subr/cmdline.c Mon Mar 16 04:11:36 2020
> @@ -1305,36 +1305,92 @@ static const char *
> escape_path(apr_pool_t *pool, const char *orig_path)
> {
>apr_size_t len, esc_len;
> - const char *path;
> - char *p, *esc_path;
> + apr_status_t status;
>
> - path = apr_pescape_shell(pool, orig_path);
> + len = strlen(orig_path);
> + esc_len = 0;
>
> - len = esc_len = 0;
> + status = apr_escape_shell(NULL, orig_path, len, _len);
>
> - /* Now that apr has done its escaping, we can check whether there's any
> - whitespace that also needs to be escaped. This must be done after the
> - fact, otherwise apr_pescape_shell() would escape the backslashes we're
> - inserting. */
> - for (p = (char *)path; *p; p++)
> + if (status == APR_NOTFOUND)
> {
> - len++;
> - if (*p == ' ' || *p == '\t')
> -esc_len++;
> + /* No special characters found by APR, so just surround it in double
> + quotes in case there is whitespace, which APR (as of 1.6.5) doesn't
> + consider special. */
> + return apr_psprintf(pool, "\"%s\"", orig_path);
> }
> -
> - if (esc_len == 0)
> -return path;
> -
> - p = esc_path = apr_pcalloc(pool, len + esc_len + 1);
> - while (*path)
> + else
> {
> - if (*path == ' ' || *path == '\t')
> -*p++ = '\\';
> - *p++ = *path++;
> -}
> +#ifdef WIN32
> + const char *p;
> + /* Following the advice from
> +
> https://docs.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way
> + 1. Surround argument with double-quotes
> + 2. Escape backslashes, if they're followed by a double-quote, and
> double-quotes
> +
Re: svn commit: r1882518 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/tests/org/apache/subversion/javahl/BasicTests.java
On Thu, Oct 15, 2020 at 12:12 PM wrote:
>
> Author: amiloslavskiy
> Date: Thu Oct 15 10:12:52 2020
> New Revision: 1882518
>
> URL: http://svn.apache.org/viewvc?rev=1882518=rev
> Log:
> JavaHL: Introduce tests showing JVM crashes with TunnelAgent
>
> The crashes will be addressed in subsequent commits.
>
> [in subversion/bindings/javahl]
> * tests/org/apache/subversion/javahl/BasicTests.java
> Add helper class 'TestTunnelAgent' to emulate server replies in test
> environment.
> Add new tests which currently causes JVM to crash:
> * testCrash_RemoteSession_nativeDispose
> * testCrash_RequestChannel_nativeRead_AfterException
> * testCrash_RequestChannel_nativeRead_AfterSvnError
A couple more small remarks here below ...
> +private String readClient(ByteBuffer readBuffer)
> + throws IOException
> + {
> + readBuffer.reset();
> + request.read(readBuffer);
> +
> + final int offset = readBuffer.arrayOffset();
> + return new String(readBuffer.array(),
> + offset,
> + readBuffer.position() - offset);
> + }
> +
> + private void emulateServer(String serverMessage)
> + throws IOException
> + {
> + final byte[] responseBytes = serverMessage.getBytes();
> + response.write(ByteBuffer.wrap(responseBytes));
> + }
^^ some lines indented with tabs instead of spaces
> +public void testCrash_RemoteSession_nativeDispose() {
^^ curly brace should be on next line
> +// 'OperationContext::openTunnel()' doesn't 'NewGlobalRef()'
> callback returned by 'TunnelAgent.openTunnel()'.
> +// When GC runs, it disposes the callback. When JavaHL tries to call
> it in 'remote.dispose()', JVM crashes.
^^ The comment above, describing how the JVM crashes, refers to the
situation before you actually fixed that :-). Can you rephrase it a
bit, so it is still applicable even with the crash now fixed?
> +public void testCrash_RequestChannel_nativeRead_AfterSvnError()
> +{
> +final String wcRoot = new File("tempSvnRepo").getAbsolutePath();
> +
> +final ScriptItem[] script = new ScriptItem[]{
> +// openRemoteSession
> +new ScriptItem(Actions.EMUL_SERVER, "( success ( 2 2 ( ) (
> edit-pipeline svndiff1 absent-entries commit-revprops depth log-revprops
> atomic-revprops partial-replay inherited-props ephemeral-txnprops
> file-revs-reverse ) ) ) "),
> +new ScriptItem(Actions.READ_CLIENT, "edit-pipeline"),
> +new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ANONYMOUS )
> 36:0113e071-0208-4a7b-9f20-3038f9caf0f0 ) ) "),
> +new ScriptItem(Actions.READ_CLIENT, "ANONYMOUS"),
> +new ScriptItem(Actions.EMUL_SERVER, "( success ( ) ) ( success (
> 36:---- 25:svn+test://localhost/test (
> mergeinfo ) ) ) "),
> +// checkout
> +new ScriptItem(Actions.READ_CLIENT, "( get-latest-rev ( ) ) "),
> +// Error causes a SubversionException to be created, which then
> +// skips closing the Tunnel properly due to 'ExceptionOccurred()'
> +// in 'OperationContext::closeTunnel()'.
> +// If TunnelAgent is unaware and calls
> 'RequestChannel.nativeRead()',
> +// it will either crash or try to use a random file.
> +new ScriptItem(Actions.EMUL_SERVER, "( success ( ( ) 0: ) ) (
> failure ( ( 160006 20:This is a test error 0: 0 ) ) ) "),
> +// TunnelAgent is not aware about the error and just keeps
> reading.
^^ same here, the comments describe the behavior before the fix, maybe
rephrase it so it's still valid after you fixed it.
--
Johan
Re: preparing to release 1.14.1 and 1.10.7
On Wed, Jan 27, 2021 at 11:03 AM Thomas Singer wrote: > > Hi Johan, > > Yes, I mean those of Alex' patches that hang around in pending state > since October. Thank you for reviewing them. > > I'm not sure whether other patches from Alex from August > > - [PATCH] Fix JavaHL crash in RequestChannel.nativeRead > - [PATCH] Fix JavaHL crash in TunnelAgent.CloseTunnelCallback after GC Ah, I think he re-introduced them in the javahl-1.14-fixes branch which I'm reviewing. So if all goes well, we should get those in (/crossing fingers). > or September > > - [PATCH] Fix JavaHL error in SVNBase::createCppBoundObject > > already were accepted. Yes, I believe that one was committed [1], and is already backported to the 1.14.x branch [2], so will be part of 1.14.1. [1] https://svn.apache.org/r1882115 [2] https://svn.apache.org/r1883407 > -- > Best regards, > Thomas Singer > > > On 2021-01-27 10:45, Johan Corveleyn wrote: > > On Wed, Jan 27, 2021 at 10:30 AM Thomas Singer > > wrote: > >> > >> Please include Alexandr Miloslavskii's JavaHL fixes with SVN 1.14.1. > >> Thanks in advance. > > > > Yep, working on it :-). I assume you're referring to the > > javahl-1.14-fixes branch, which he pointed to in the dev@ thread with > > subject "A series of JavaHL fixes on branch 'javahl-1.14-fixes'"? Or > > are there other fixes that slipped through the cracks? > > > > I agree we should try to get them in, if possible. I hope to wrap up > > my review today. > > -- Johan
Re: A series of JavaHL fixes on branch 'javahl-1.14-fixes'
On Wed, Jan 27, 2021 at 2:40 AM Johan Corveleyn wrote: > > On Tue, Jan 26, 2021 at 9:56 PM Johan Corveleyn wrote: > > > > On Tue, Jan 26, 2021 at 7:43 PM Alexandr Miloslavskiy > > wrote: > > > > > > On 25.01.2021 1:55, Johan Corveleyn wrote: > > > > > > > As an update: I've started looking into this, but it's a bit more work > > > > than I anticipated. I want to understand and properly review the > > > > changes. > > > > > > Thanks! I already started thinking that all hope is lost :) > > > > > > The branch contains additional tests (added over a few commits). If you > > > keep the tests but discard my patches to JavaHL, you'll see the crashes. > > > With patches, crashes are no more. > > > > > > I tried to properly explain everything in code comments and commit > > > messages. However, if you have any questions, please ask, and I'll do my > > > best to explain. > > > > Great! Thanks for sticking around. I'll try to get through it all :-). > > Okay, I went through it all, and it looks good to me. Those are some > pretty delicate bugs / fixes, so thanks for explaining everything so > well in the comments and commit messages. Great work! > > Apart from reading the code, I've applied the tests and their fixes > individually to better understand them, and to make sure they first > failed on my system and were subsequently fixed by the corresponding > fix. After that, I'm satisfied that they are a good step forward. > > I did have a couple of tiny questions / remarks, but I'm going to get > back to them tomorrow, after another fresh look. Done. @Alexandr: if you could please take a look at the small remarks / questions I raised on r1882520, r1882522 and r1882523, that would be great. Thanks, -- Johan
Re: svn commit: r1882523 - in /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl: native/ src/org/apache/subversion/javahl/util/
On Thu, Oct 15, 2020 at 12:16 PM wrote:
>
> Author: amiloslavskiy
> Date: Thu Oct 15 10:16:39 2020
> New Revision: 1882523
>
> URL: http://svn.apache.org/viewvc?rev=1882523=rev
> Log:
> JavaHL: Fix crash when TunnelAgent continues reading after being closed
>
> The problem here is that when 'RemoteSession' is destroyed, it also
> does 'apr_pool_destroy()' which frees memory behind 'apr_file_t', which
> is represented by 'TunnelChannel.nativeChannel' in Java.
> 'TunnelChannel' runs on a different thread and is unaware that
> 'apr_file_t' pointer is now invalid.
>
> Fix this by informing 'TunnelChannel' before 'apr_file_t' is freed.
>
> This crash is demonstrated by the following JavaHL tests:
> * testCrash_RequestChannel_nativeRead_AfterException
> * testCrash_RequestChannel_nativeRead_AfterSvnError
>
> This commit alone does not fix all problems in these tests, see
> subsequent commits as well.
>
> [in subversion/bindings/javahl]
> * native/OperationContext.cpp
> (close_TunnelChannel): New function to inform Java side.
> (openTunnel): Keep references to Java tunnel objects.
> (closeTunnel): Inform Java side when tunnel is closed.
> * src/org/apache/subversion/javahl/util/RequestChannel.java
> * src/org/apache/subversion/javahl/util/ResponseChannel.java
> Add 'synchronized' to avoid error described in 'syncClose()'
> * src/org/apache/subversion/javahl/util/Tunnel.java
> A new function to clean up. I decided not to change old '.close()'
> because the new function can lead to a deadlock if used incorrectly.
>
> Modified:
>
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
>
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/RequestChannel.java
>
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/ResponseChannel.java
>
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/src/org/apache/subversion/javahl/util/TunnelChannel.java
>
> Modified:
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
> URL:
> http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp?rev=1882523=1882522=1882523=diff
> ==
> ---
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
> (original)
> +++
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
> Thu Oct 15 10:16:39 2020
> @@ -492,6 +492,8 @@ public:
>request_out(NULL),
>response_in(NULL),
>response_out(NULL),
> + jrequest(NULL),
> + jresponse(NULL),
>jclosecb(NULL)
> {
>status = apr_file_pipe_create_ex(_in, _out,
> @@ -512,6 +514,8 @@ public:
>apr_file_t *response_in;
>apr_file_t *response_out;
>apr_status_t status;
> + jobject jrequest;
> + jobject jresponse;
These new fields are not mentioned in the log message.
>jobject jclosecb;
> };
>
> @@ -523,7 +527,10 @@ jobject create_Channel(const char *class
>jmethodID ctor = env->GetMethodID(cls, "", "(J)V");
>if (JNIUtil::isJavaExceptionThrown())
> return NULL;
> - return env->NewObject(cls, ctor, reinterpret_cast(fd));
> + jobject channel = env->NewObject(cls, ctor, reinterpret_cast(fd));
> + if (JNIUtil::isJavaExceptionThrown())
> +return NULL;
> + return env->NewGlobalRef(channel);
> }
^^ also not mentioned in log message.
(rest looks fine, so if you could amend the commit message with the
missing details, that'd be great)
--
Johan
Re: svn commit: r1882522 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
On Thu, Oct 15, 2020 at 12:16 PM wrote:
>
> Author: amiloslavskiy
> Date: Thu Oct 15 10:15:47 2020
> New Revision: 1882522
>
> URL: http://svn.apache.org/viewvc?rev=1882522=rev
> Log:
> JavaHL: Fix crash in TunnelAgent.CloseTunnelCallback after GC
>
> When jobject reference is kept across different JNI calls, a new global
> reference must be requested with NewGlobalRef(). Otherwise, GC is free
> to remove the object. Even if Java code keeps a reference to the object,
> GC can still move the object around, invalidating the kept jobject,
> which results in a native crash when trying to access it.
>
> This crash is demonstrated by the following JavaHL test:
> 'testCrash_RemoteSession_nativeDispose'
>
> [in subversion/bindings/javahl]
> * native/OperationContext.cpp
> (callCloseTunnelCallback): Extract function to facilitate changes in
> further commits.
> (openTunnel): Add NewGlobalRef() for kept jobject.
> (callCloseTunnelCallback): Add a matching DeleteGlobalRef().
>
> Modified:
>
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
>
> Modified:
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
> URL:
> http://svn.apache.org/viewvc/subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp?rev=1882522=1882521=1882522=diff
> ==
> ---
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
> (original)
> +++
> subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/OperationContext.cpp
> Thu Oct 15 10:15:47 2020
> @@ -629,23 +629,17 @@ OperationContext::openTunnel(svn_stream_
>jtunnel_name, juser, jhostname, jint(port)),
>SVN_ERR_BASE);
>
> + if (tc->jclosecb)
> +{
> + tc->jclosecb = env->NewGlobalRef(tc->jclosecb);
> + SVN_JNI_CATCH(, SVN_ERR_BASE);
> +}
> +
>return SVN_NO_ERROR;
> }
>
> -void
> -OperationContext::closeTunnel(void *tunnel_context, void *)
> +void callCloseTunnelCallback(JNIEnv* env, jobject jclosecb)
> {
> - TunnelContext* tc = static_cast(tunnel_context);
> - jobject jclosecb = tc->jclosecb;
> - delete tc;
> -
> - if (!jclosecb)
> -return;
> -
> - JNIEnv *env = JNIUtil::getEnv();
> - if (JNIUtil::isJavaExceptionThrown())
> -return;
> -
>static jmethodID mid = 0;
>if (0 == mid)
> {
> @@ -656,4 +650,20 @@ OperationContext::closeTunnel(void *tunn
>SVN_JNI_CATCH_VOID(mid = env->GetMethodID(cls, "closeTunnel", "()V"));
> }
>SVN_JNI_CATCH_VOID(env->CallVoidMethod(jclosecb, mid));
> + env->DeleteGlobalRef(jclosecb);
> +}
> +
> +void
> +OperationContext::closeTunnel(void *tunnel_context, void *)
> +{
> + TunnelContext* tc = static_cast(tunnel_context);
> + jobject jclosecb = tc->jclosecb;
> + delete tc;
> +
> + JNIEnv *env = JNIUtil::getEnv();
> + if (JNIUtil::isJavaExceptionThrown())
> +return;
> +
> + if (jclosecb)
> +callCloseTunnelCallback(env, jclosecb);
> }
>
>
Here above in closeTunnel(), you dropped the early exit on if
(!jclosecb) (before the call to JNIUtil::getEnv()). Was this
intentional?
Previously, in the case of a null jclosecb, the unnecessary calls to
JNIUtil::getEnv() and JNIUtil::isJavaExceptionThrown() were also
avoided. In the new version they aren't (not sure if that's important,
but it's a small behavioral difference).
--
Johan
Re: svn commit: r1882520 - /subversion/branches/javahl-1.14-fixes/subversion/bindings/javahl/native/JNIUtil.cpp
On Thu, Oct 15, 2020 at 12:14 PM wrote: > > Author: amiloslavskiy > Date: Thu Oct 15 10:14:30 2020 > New Revision: 1882520 > > URL: http://svn.apache.org/viewvc?rev=1882520=rev > Log: > JavaHL: Fix 'JNI call made with exception pending' error > > It is incorrect to call Java method with 'CallObjectMethod' when there > is an unhandled Java exception already pending. The fix is to > temporarily move exception out of the way. > > This error is easily seen when running JavaHL tests. Hi Alexandr, I agree with this change, but was wondering what you meant by "This error is easily seen when running JavaHL tests"? Is there a warning or ...? Can you explain how to see the effect of "the absence of this fix"? Same remark / question for your next commit, r1882521, where you say in the commit message: "These warnings are seen in JavaHL tests due to '-Xcheck:jni' option." For context: I'm building on Windows, with AdoptOpenJDK-11.0.6.10. I have performed a "release" build, but have adjusted win-tests.py to pass the -Xcheck:jni option. I'm not seeing any warnings that point to this, when running the javahl testsuite. I am however seeing a bunch of other warnings, but I assume these are totally unrelated to the work you did on the javahl-1.14-fixes branch: I do get 8 warnings with this sort of message (always similar stacktrace, always coming from UtilTests.testFileMerge, line 120): [[[ WARNING: JNI local refs: 33, exceeds capacity: 32 at java.net.NetworkInterface.getAll(java.base@11.0.6/Native Method) at java.net.NetworkInterface.getNetworkInterfaces(java.base@11.0.6/NetworkInterface.java:359) at sun.security.provider.SeedGenerator.addNetworkAdapterInfo(java.base@11.0.6/SeedGenerator.java:229) at sun.security.provider.SeedGenerator$1.run(java.base@11.0.6/SeedGenerator.java:179) at sun.security.provider.SeedGenerator$1.run(java.base@11.0.6/SeedGenerator.java:167) at java.security.AccessController.doPrivileged(java.base@11.0.6/Native Method) at sun.security.provider.SeedGenerator.getSystemEntropy(java.base@11.0.6/SeedGenerator.java:167) at sun.security.provider.AbstractDrbg$SeederHolder.(java.base@11.0.6/AbstractDrbg.java:551) at sun.security.provider.AbstractDrbg.getEntropyInput(java.base@11.0.6/AbstractDrbg.java:505) at sun.security.provider.AbstractDrbg.getEntropyInput(java.base@11.0.6/AbstractDrbg.java:494) at sun.security.provider.AbstractDrbg.instantiateIfNecessary(java.base@11.0.6/AbstractDrbg.java:696) - locked <0x8a727f88> (a sun.security.provider.HashDrbg) at sun.security.provider.AbstractDrbg.engineNextBytes(java.base@11.0.6/AbstractDrbg.java:378) at sun.security.provider.AbstractDrbg.engineNextBytes(java.base@11.0.6/AbstractDrbg.java:334) at sun.security.provider.DRBG.engineNextBytes(java.base@11.0.6/DRBG.java:233) at java.security.SecureRandom.nextBytes(java.base@11.0.6/SecureRandom.java:741) at java.security.SecureRandom.next(java.base@11.0.6/SecureRandom.java:798) at java.util.Random.nextLong(java.base@11.0.6/Random.java:424) at java.io.File$TempDirectory.generateFile(java.base@11.0.6/File.java:1930) at java.io.File.createTempFile(java.base@11.0.6/File.java:2078) at org.apache.subversion.javahl.UtilTests.testFileMerge(UtilTests.java:120) at jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(java.base@11.0.6/Native Method) at jdk.internal.reflect.NativeMethodAccessorImpl.invoke(java.base@11.0.6/NativeMethodAccessorImpl.java:62) at jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(java.base@11.0.6/DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(java.base@11.0.6/Method.java:566) at junit.framework.TestCase.runTest(TestCase.java:176) at junit.framework.TestCase.runBare(TestCase.java:141) at junit.framework.TestResult$1.protect(TestResult.java:122) at junit.framework.TestResult.runProtected(TestResult.java:142) at junit.framework.TestResult.run(TestResult.java:125) at junit.framework.TestCase.run(TestCase.java:129) at junit.framework.TestSuite.runTest(TestSuite.java:252) at junit.framework.TestSuite.run(TestSuite.java:247) at junit.framework.TestSuite.runTest(TestSuite.java:252) at junit.framework.TestSuite.run(TestSuite.java:247) at junit.textui.TestRunner.doRun(TestRunner.java:116) at junit.textui.TestRunner.doRun(TestRunner.java:109) at junit.textui.TestRunner.run(TestRunner.java:77) at org.apache.subversion.javahl.RunTests.main(RunTests.java:119) ]]] I suppose I have some lower default limit for these JNI local refs ... perhaps this should be adjusted somewhere in our test suite (maybe this was already done for unix, but not for Windows, but I haven't investigated further) Thanks, -- Johan
Re: preparing to release 1.14.1 and 1.10.7
On Wed, Jan 27, 2021 at 10:30 AM Thomas Singer wrote: > > Please include Alexandr Miloslavskii's JavaHL fixes with SVN 1.14.1. > Thanks in advance. Yep, working on it :-). I assume you're referring to the javahl-1.14-fixes branch, which he pointed to in the dev@ thread with subject "A series of JavaHL fixes on branch 'javahl-1.14-fixes'"? Or are there other fixes that slipped through the cracks? I agree we should try to get them in, if possible. I hope to wrap up my review today. -- Johan
Re: A series of JavaHL fixes on branch 'javahl-1.14-fixes'
On Tue, Jan 26, 2021 at 9:56 PM Johan Corveleyn wrote: > > On Tue, Jan 26, 2021 at 7:43 PM Alexandr Miloslavskiy > wrote: > > > > On 25.01.2021 1:55, Johan Corveleyn wrote: > > > > > As an update: I've started looking into this, but it's a bit more work > > > than I anticipated. I want to understand and properly review the > > > changes. > > > > Thanks! I already started thinking that all hope is lost :) > > > > The branch contains additional tests (added over a few commits). If you > > keep the tests but discard my patches to JavaHL, you'll see the crashes. > > With patches, crashes are no more. > > > > I tried to properly explain everything in code comments and commit > > messages. However, if you have any questions, please ask, and I'll do my > > best to explain. > > Great! Thanks for sticking around. I'll try to get through it all :-). Okay, I went through it all, and it looks good to me. Those are some pretty delicate bugs / fixes, so thanks for explaining everything so well in the comments and commit messages. Great work! Apart from reading the code, I've applied the tests and their fixes individually to better understand them, and to make sure they first failed on my system and were subsequently fixed by the corresponding fix. After that, I'm satisfied that they are a good step forward. I did have a couple of tiny questions / remarks, but I'm going to get back to them tomorrow, after another fresh look. -- Johan
Re: preparing to release 1.14.1 and 1.10.7
On Tue, Jan 26, 2021 at 11:47 PM Stefan Sperling wrote: > > On Tue, Jan 26, 2021 at 04:45:32PM -0500, Nathan Hartman wrote: > > On Tue, Jan 26, 2021 at 4:28 PM Daniel Shahaf > > wrote: > > > > > Stefan Sperling wrote on Fri, Jan 22, 2021 at 14:36:56 +0100: > > > > [...] I will allow myself to merge any nominations which received at > > > least > > > > two +1 votes and no vetoes. > > > > > > HACKING [1] states: > > > > > > "For an LTS release line, a change is approved if it receives three +1s > > > and no > > > vetoes. (Only binding votes count; see above.)" > > > > > > If you want to change the policy, do so in a new thread with an > > > appropriate > > > subject line. > > > > > > I read that as meaning that stsp will vote on items that need a third vote. > > > > Nathan > > I literally meant that if there turned out to be lack of voting activity > with important-looking fixes still in STATUS, I would just merge them with > two +1s instead of three, applying the "silence implies consensus" principle. > > But I'll follow Daniel's opinion now and will simply leave any such fixes > dangling. Yes, I agree we should stick to our established policy here. We recently loosened our backport-voting policy a bit for non-LTS (Regular) releases, but we didn't for LTS releases (no time to dig up the thread, but ISTR we discussed it a bit, but didn't reach consensus on that). I know it's tempting to loosen even more, with the ever decreasing developer / volunteer activity, but we shouldn't do so without proper discussion about our policy. If we really start running into backport-vote-shortage for important fixes we should probably highlight this in our next board report. Anyway, so far, for 1.14.1 and 1.10.7 we seem to be getting just enough votes on most backport proposals (albeit just barely, and after months of waiting in some cases). If any are still in there that you (Stefan) or anyone feels should really get more votes, please highlight them a bit on the mailinglist ... maybe it's not too late to convince some more people to join the effort. -- Johan
