You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Branko Čibej <br...@xbc.nu> on 2003/11/03 07:00:40 UTC
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
brane@xbc.nu wrote:
>BUILD: Revision 7611 on i386-unknown-freebsdelf5.0 static
>
>
...
>Running all tests in merge_tests.py...FAILURE
>
>
(fails everywhere else, too)
I think this is a consequence of the move to use apr_tmp_dir_get in
revision 7609. Both of my test machines have /tmp and /home on mounted
on different volumes; now apr_file_renams on Unix just calls rename(2),
which does not support cross-volume renames and returns EXDEV instead.
I think this is an APR misfeature. The docs on apr_file_rename say that
"Moving files or directories across devices may not be possible," but I
think the workaround is so simple that APR should just implement it
internally The Windows implementation of apr_file_rename does revert to
copy+delete on cross-volume renames.
Thoughts?
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Sander Striker <st...@apache.org>.
> From: Branko Cibej [mailto:brane@xbc.nu]
> Sent: Monday, November 03, 2003 8:01 AM
> brane@xbc.nu wrote:
>
> >BUILD: Revision 7611 on i386-unknown-freebsdelf5.0 static
> >
> >
> ...
>
> >Running all tests in merge_tests.py...FAILURE
> >
> >
> (fails everywhere else, too)
>
> I think this is a consequence of the move to use apr_tmp_dir_get in
> revision 7609. Both of my test machines have /tmp and /home on mounted
> on different volumes; now apr_file_renams on Unix just calls rename(2),
> which does not support cross-volume renames and returns EXDEV instead.
Oof. I don't seem to be able to commit something without fall out lately... :(
> I think this is an APR misfeature. The docs on apr_file_rename say that
> "Moving files or directories across devices may not be possible," but I
> think the workaround is so simple that APR should just implement it
> internally The Windows implementation of apr_file_rename does revert to
> copy+delete on cross-volume renames.
>
> Thoughts?
Agree.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Russell Yanofsky <re...@columbia.edu>.
Sander Striker wrote:
> ...
> I think that requiring APR_0_9_BRANCH on win32 temporarily might be
> acceptable. Thoughts?
Sure it's acceptable. What makes it desirable? It's not like this problem is
very hard to fix, there's a patch...
- Russ
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Russell Yanofsky <re...@columbia.edu>.
Branko Čibej wrote:
> Russell Yanofsky wrote:
>
>> Branko Čibej wrote:
>>
>>
>>> ...
>>> Since it's temporary, I think we can live without the helper
>>> function. All we gain with the helper is two less string
>>> conversions ...
>>>
>>>
>>
>> How about, "Since it's temporary, let's leave well enough alone and
>> go respond to Russ's message"?
>>
>> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=48883
>>
>> :)
>>
>>
> That message is irrelevant. It has nothing to do with this thread.
Sure it does, you must not have noticed my clever transition. But whether or
not it's irrelevant to this thread, I don't think it's irrelevant in and of
itself. If you think it is, please say why in your response to my message.
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=48883
:)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Sander Striker <st...@apache.org>.
> From: D.J. Heap [mailto:djheap@dhiprovo.com]
> Sent: Monday, November 03, 2003 7:13 PM
[...]
> With Brane's patch to apu.hw (in both places -- httpd's directory and
> Subversion's copy of apr-util) and this little patch I'm compiling fine
> on Windows now.
What little patch? *whistles* Applied in 7618. Thanks.
> It's not ideal, but since it is temporary, I personally don't mind.
I have the same sentiments. Also, I wonder how many people actually
build from source on windows. I bet it's a lot less than before the
installer was around.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by "D.J. Heap" <dj...@dhiprovo.com>.
Sander Striker wrote:
>>From: Branko Cibej [mailto:brane@xbc.nu]
>>Sent: Monday, November 03, 2003 5:52 PM
>
>
>>>How about, "Since it's temporary, let's leave well enough alone and go
>>>respond to Russ's message"?
>>>
>>>http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=48883
>>>
>>>:)
>>
>>That message is irrelevant. It has nothing to do with this thread.
>
>
> And TBH, I'd rather not go that way. Unless a large crowd of people
> compiling on win32 decides on grouping and purchasing pitchforks.
>
> I think that requiring APR_0_9_BRANCH on win32 temporarily might be
> acceptable. Thoughts?
>
> Sander
>
With Brane's patch to apu.hw (in both places -- httpd's directory and
Subversion's copy of apr-util) and this little patch I'm compiling fine
on Windows now. It's not ideal, but since it is temporary, I personally
don't mind.
DJ
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 7614)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -273,7 +273,7 @@
}
#ifdef SVN_WIN32
/* Next, on Win32, try the C:\TEMP directory. */
- if (test_tempdir("C:\\TEMP", p))
+ if (test_tempdir("C:\\TEMP", pool))
{
memcpy(global_temp_dir, "C:\\TEMP", 7 + 1);
goto end;
**********************************************************************
This email and any files transmitted with it are confidential
and intended solely for the use of the individual or entity to
whom they are addressed. If you have received this email
in error please notify the system manager.
This footnote also confirms that this email message has been
swept by MIMEsweeper for the presence of computer viruses.
www.mimesweeper.com
**********************************************************************
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Sander Striker <st...@apache.org>.
> From: Branko Cibej [mailto:brane@xbc.nu]
> Sent: Monday, November 03, 2003 5:52 PM
>> How about, "Since it's temporary, let's leave well enough alone and go
>> respond to Russ's message"?
>>
>> http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=48883
>>
>> :)
>
> That message is irrelevant. It has nothing to do with this thread.
And TBH, I'd rather not go that way. Unless a large crowd of people
compiling on win32 decides on grouping and purchasing pitchforks.
I think that requiring APR_0_9_BRANCH on win32 temporarily might be
acceptable. Thoughts?
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Branko Čibej <br...@xbc.nu>.
Russell Yanofsky wrote:
>Branko Čibej wrote:
>
>
>>...
>>Since it's temporary, I think we can live without the helper function.
>>All we gain with the helper is two less string conversions ...
>>
>>
>
>How about, "Since it's temporary, let's leave well enough alone and go
>respond to Russ's message"?
>
>http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=48883
>
>:)
>
>
That message is irrelevant. It has nothing to do with this thread.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Russell Yanofsky <re...@columbia.edu>.
Branko Čibej wrote:
> ...
> Since it's temporary, I think we can live without the helper function.
> All we gain with the helper is two less string conversions ...
How about, "Since it's temporary, let's leave well enough alone and go
respond to Russ's message"?
http://subversion.tigris.org/servlets/ReadMsg?list=dev&msgNo=48883
:)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Branko Čibej <br...@xbc.nu>.
Sander Striker wrote:
>>From: Branko Cibej [mailto:brane@xbc.nu]
>>Sent: Monday, November 03, 2003 8:01 AM
>>
>>
>>brane@xbc.nu wrote:
>>
>>
>>
>>>BUILD: Revision 7611 on i386-unknown-freebsdelf5.0 static
>>>
>>>
>>...
>>
>>
>>
>>>Running all tests in merge_tests.py...FAILURE
>>>
>>>
>>(fails everywhere else, too)
>>
>>I think this is a consequence of the move to use apr_tmp_dir_get in
>>revision 7609. Both of my test machines have /tmp and /home on mounted
>>on different volumes; now apr_file_renams on Unix just calls rename(2),
>>which does not support cross-volume renames and returns EXDEV instead.
>>
>>I think this is an APR misfeature. The docs on apr_file_rename say that
>>"Moving files or directories across devices may not be possible," but I
>>think the workaround is so simple that APR should just implement it
>>internally The Windows implementation of apr_file_rename does revert to
>>copy+delete on cross-volume renames.
>>
>>Thoughts?
>>
>>
>
>Something like this? (needs win32 counterpart)
>
>Index: file_io/unix/open.c
>===================================================================
>RCS file: /home/cvs/apr/file_io/unix/open.c,v
>retrieving revision 1.111
>diff -u -r1.111 open.c
>--- file_io/unix/open.c 19 Mar 2003 03:45:42 -0000 1.111
>+++ file_io/unix/open.c 3 Nov 2003 12:18:47 -0000
>@@ -216,7 +216,17 @@
> const char *to_path,
> apr_pool_t *p)
> {
>+ apr_status_t rv;
>+
> if (rename(from_path, to_path) != 0) {
>+ if (errno == EXDEV) {
>+ rv = apr_file_copy(from_path, to_path, APR_FILE_SOURCE_PERMS, p);
>+ if (rv)
>+ return rv;
>+
>+ return apr_file_remove(from_path, p);
>+ }
>+
> return errno;
> }
> return APR_SUCCESS;
>
>
Yup, that's what I had in mind. Of course, the note in the docstring
should go away, too.
>And temporarily in Subversion, until APR with such a fix is released:
>
>* subversion/libsvn_subr/io.c
>
> (file_copy): New helper function.
>
> (svn_io_copy_file): Use helper.
>
> (svn_io_rename_file): If rename isn't possible because it is cross filesystem,
> copy and delete instead. Use helper.
>
>
Since it's temporary, I think we can live without the helper function.
All we gain with the helper is two less string conversions, which I
think shouldn't matter at this point. Or maybe it does; if the next APR
release is far in the future?
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Branko Čibej <br...@xbc.nu>.
Sander Striker wrote:
>>From: Philip Martin [mailto:philip@codematters.co.uk]
>>Sent: Monday, November 03, 2003 7:00 PM
>>
>>
>>Subversion relies on the atomic nature of renames, although I note
>>that the APR documentation doesn't provide any such guarantee
>>directly. On Unix, at least, it is implied by the warning about
>>crossing devices.
>>
>>
>
>Since we don't want to break any other apps that rely on the atomicity
>of apr_file_rename, we'd better not change that. FWIW, I think it should
>be safe to document apr_file_name() as atomic.
>
>
It won't be atomic on Windows in the cross-volume case, when MoveFileEx
will do a copy+delete. Saying that the rename is atomic would be misleading.
--
Brane Čibej <br...@xbc.nu> http://www.xbc.nu/brane/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Sander Striker <st...@apache.org>.
> From: Philip Martin [mailto:philip@codematters.co.uk]
> Sent: Monday, November 03, 2003 7:00 PM
[...]
> The problem with a copy and delete implementation is that the copy can
> fail (or be interrupted) part way through, in which case the original
> destination file may be lost. A traditional Unix rename guarantees
> that the destination is either unmodified or replaced.
>
> Perhaps the copy should be made to a temporary file in the destination
> directory, then once the copy is complete the temporary file could be
> renamed, followed by deletion of the temporary or source as
> appropriate.
This makes sense. Also makes sense to keep in in Subversion then.
> Subversion relies on the atomic nature of renames, although I note
> that the APR documentation doesn't provide any such guarantee
> directly. On Unix, at least, it is implied by the warning about
> crossing devices.
Since we don't want to break any other apps that rely on the atomicity
of apr_file_rename, we'd better not change that. FWIW, I think it should
be safe to document apr_file_name() as atomic.
Sander
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Philip Martin <ph...@codematters.co.uk>.
"Sander Striker" <st...@apache.org> writes:
> Index: file_io/unix/open.c
> ===================================================================
> RCS file: /home/cvs/apr/file_io/unix/open.c,v
> retrieving revision 1.111
> diff -u -r1.111 open.c
> --- file_io/unix/open.c 19 Mar 2003 03:45:42 -0000 1.111
> +++ file_io/unix/open.c 3 Nov 2003 12:18:47 -0000
> @@ -216,7 +216,17 @@
> const char *to_path,
> apr_pool_t *p)
> {
> + apr_status_t rv;
> +
> if (rename(from_path, to_path) != 0) {
> + if (errno == EXDEV) {
> + rv = apr_file_copy(from_path, to_path, APR_FILE_SOURCE_PERMS, p);
> + if (rv)
> + return rv;
> +
> + return apr_file_remove(from_path, p);
> + }
> +
> return errno;
> }
> return APR_SUCCESS;
The problem with a copy and delete implementation is that the copy can
fail (or be interrupted) part way through, in which case the original
destination file may be lost. A traditional Unix rename guarantees
that the destination is either unmodified or replaced.
Perhaps the copy should be made to a temporary file in the destination
directory, then once the copy is complete the temporary file could be
renamed, followed by deletion of the temporary or source as
appropriate.
Subversion relies on the atomic nature of renames, although I note
that the APR documentation doesn't provide any such guarantee
directly. On Unix, at least, it is implied by the warning about
crossing devices.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
RE: svn rev 7611: FAIL (i386-unknown-freebsdelf5.0 static ra_local)
Posted by Sander Striker <st...@apache.org>.
> From: Branko Cibej [mailto:brane@xbc.nu]
> Sent: Monday, November 03, 2003 8:01 AM
> brane@xbc.nu wrote:
>
> >BUILD: Revision 7611 on i386-unknown-freebsdelf5.0 static
> >
> >
> ...
>
> >Running all tests in merge_tests.py...FAILURE
> >
> >
> (fails everywhere else, too)
>
> I think this is a consequence of the move to use apr_tmp_dir_get in
> revision 7609. Both of my test machines have /tmp and /home on mounted
> on different volumes; now apr_file_renams on Unix just calls rename(2),
> which does not support cross-volume renames and returns EXDEV instead.
>
> I think this is an APR misfeature. The docs on apr_file_rename say that
> "Moving files or directories across devices may not be possible," but I
> think the workaround is so simple that APR should just implement it
> internally The Windows implementation of apr_file_rename does revert to
> copy+delete on cross-volume renames.
>
> Thoughts?
Something like this? (needs win32 counterpart)
Index: file_io/unix/open.c
===================================================================
RCS file: /home/cvs/apr/file_io/unix/open.c,v
retrieving revision 1.111
diff -u -r1.111 open.c
--- file_io/unix/open.c 19 Mar 2003 03:45:42 -0000 1.111
+++ file_io/unix/open.c 3 Nov 2003 12:18:47 -0000
@@ -216,7 +216,17 @@
const char *to_path,
apr_pool_t *p)
{
+ apr_status_t rv;
+
if (rename(from_path, to_path) != 0) {
+ if (errno == EXDEV) {
+ rv = apr_file_copy(from_path, to_path, APR_FILE_SOURCE_PERMS, p);
+ if (rv)
+ return rv;
+
+ return apr_file_remove(from_path, p);
+ }
+
return errno;
}
return APR_SUCCESS;
And temporarily in Subversion, until APR with such a fix is released:
* subversion/libsvn_subr/io.c
(file_copy): New helper function.
(svn_io_copy_file): Use helper.
(svn_io_rename_file): If rename isn't possible because it is cross filesystem,
copy and delete instead. Use helper.
Index: subversion/libsvn_subr/io.c
===================================================================
--- subversion/libsvn_subr/io.c (revision 7614)
+++ subversion/libsvn_subr/io.c (working copy)
@@ -329,43 +329,26 @@
^L
/*** Creating, copying and appending files. ***/
-svn_error_t *
-svn_io_copy_file (const char *src,
- const char *dst,
- svn_boolean_t copy_perms,
- apr_pool_t *pool)
+static svn_error_t *
+file_copy (const char *src,
+ const char *src_apr,
+ const char *dst,
+ const char *dst_apr,
+ svn_boolean_t copy_perms,
+ apr_pool_t *pool)
{
- apr_file_t *d;
apr_status_t apr_err;
- const char *src_apr, *dst_tmp_apr;
- const char *dst_tmp;
- SVN_ERR (svn_path_cstring_from_utf8 (&src_apr, src, pool));
-
- /* For atomicity, we translate to a tmp file and then rename the tmp
- file over the real destination. */
-
- SVN_ERR (svn_io_open_unique_file (&d, &dst_tmp, dst, ".tmp", FALSE, pool));
- SVN_ERR (svn_path_cstring_from_utf8 (&dst_tmp_apr, dst_tmp, pool));
-
- apr_err = apr_file_close (d);
+ apr_err = apr_file_copy (src_apr, dst_apr, APR_OS_DEFAULT, pool);
if (apr_err)
- {
- return svn_error_createf
- (apr_err, NULL,
- "svn_io_copy_file: error closing '%s'", dst_tmp);
- }
-
- apr_err = apr_file_copy (src_apr, dst_tmp_apr, APR_OS_DEFAULT, pool);
- if (apr_err)
return svn_error_createf
(apr_err, NULL, "svn_io_copy_file: error copying '%s' to '%s'",
- src, dst_tmp);
+ src, dst);
/* If copying perms, set the perms on dst_tmp now, so they will be
atomically inherited in the upcoming rename. But note that we
had to wait until now to set perms, because if they say
- read-only, then we'd have failed filling dst_tmp's contents. */
+ read-only, then we'd have failed filling dst's contents. */
/* ### FIXME: apr_file_copy with perms may fail on Win32. We need a
platform-specific implementation to get the permissions right. */
@@ -396,7 +379,7 @@
(apr_err, NULL,
"svn_io_copy_file: closing '%s' after reading perms", src);
- apr_err = apr_file_perms_set (dst_tmp_apr, finfo.protection);
+ apr_err = apr_file_perms_set (dst_apr, finfo.protection);
/* We shouldn't be able to get APR_INCOMPLETE or APR_ENOTIMPL
here under normal circumstances, because the perms themselves
@@ -409,11 +392,43 @@
{
return svn_error_createf
(apr_err, NULL,
- "svn_io_copy_file: setting perms on '%s'", dst_tmp);
+ "svn_io_copy_file: setting perms on '%s'", dst);
}
}
#endif /* ! SVN_WIN32 */
+ return SVN_NO_ERROR;
+}
+
+svn_error_t *
+svn_io_copy_file (const char *src,
+ const char *dst,
+ svn_boolean_t copy_perms,
+ apr_pool_t *pool)
+{
+ apr_file_t *d;
+ apr_status_t apr_err;
+ const char *src_apr, *dst_tmp_apr;
+ const char *dst_tmp;
+
+ SVN_ERR (svn_path_cstring_from_utf8 (&src_apr, src, pool));
+
+ /* For atomicity, we translate to a tmp file and then rename the tmp
+ file over the real destination. */
+
+ SVN_ERR (svn_io_open_unique_file (&d, &dst_tmp, dst, ".tmp", FALSE, pool));
+ SVN_ERR (svn_path_cstring_from_utf8 (&dst_tmp_apr, dst_tmp, pool));
+
+ apr_err = apr_file_close (d);
+ if (apr_err)
+ {
+ return svn_error_createf
+ (apr_err, NULL,
+ "svn_io_copy_file: error closing '%s'", dst_tmp);
+ }
+
+ SVN_ERR (file_copy (src, src_apr, dst_tmp, dst_tmp_apr, copy_perms, pool));
+
return svn_io_file_rename (dst_tmp, dst, pool);
}
@@ -1747,6 +1762,13 @@
status = apr_file_rename (from_path_apr, to_path_apr, pool);
+ if (APR_STATUS_IS_EXDEV(status))
+ {
+ SVN_ERR (file_copy (from_path, from_path_apr, to_path, to_path_apr, TRUE, pool));
+ SVN_ERR (svn_io_remove_file (from_path, pool));
+ return SVN_NO_ERROR;
+ }
+
#ifdef SVN_WIN32
/*
Windows is 'aided' by a number of types of applications that
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org