You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Greg Hudson <gh...@MIT.EDU> on 2004/05/01 17:27:30 UTC
FS abstraction and circular dependencies
When I wrote the FS abstraction code, I deliberately introduced some
circular dependencies between the FS modules and the loader library,
figuring that it would work out as long as the loader library
consisted of only one source file.
Well, that castle has apparently come crumbling down; reportedly,
Windows doesn't allow shared libraries to have undefined symbols, so
you can only have circular references with a static build.
The circular dependencies consist of:
* The node-rev ID functions, some part of the svn_fs API and some
internal, which (as previously mentioned) are implemented in the
loader library.
* A utility function svn_fs__canonicalize_path, which I happened to
notice when making a pass over libsvn_fs_base (because it was in
fs.c, which I devoted more attention to). It's not a tremendous
amount of code, but it's possible that there are other
FS-independent internal utility functions which could be factored
out.
Here are possible solutions:
* Introduce a fourth library, libsvn_fs_util, containing the
node-rev ID functions and svn_fs__canonicalize_path.
* Stuff these functions into libsvn_subr, using internal names.
(Same idea as the previous one, trading off scope elegance against
the overhead of creating a new small library.)
* Move svn_fs__canonicalize_path back into the individual filesystem
modules and work harder at vtable-izing the node-rev ID functions,
so that there is no need for circular dependencies. I made two
proposals on how to do this in
http://www.contactor.se/~dast/svn/archive-2004-04/1461.shtml,
although none of them are terribly attractive.
* When loading the FS module, pass a pointer to a vtable containing
the node-rev ID and utility functions located in the loader
library.
* Move svn_fs__canonicalize_path back into the FS modules, and
simply duplicate the node-rev ID functions (using internal names)
in each FS module. When svn 2.0 rolls around and we can
vtable-ize the node-rev ID functions the easy way, we can
eliminate the loader library implementation of those functions.
Preferences? I think I side with the last option at the moment, but
my opinion is fluid.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Max Bowsher <ma...@ukf.net>.
Greg Hudson wrote:
> On Sat, 2004-05-01 at 18:17, Max Bowsher wrote:
>> Attached is a first draft patch, although I've called it libsvn_fs_subr.
>
> Hm. An important thing to call out about your patch is that
> libsvn_fs_subr implements some, but not all, of the FS API, in order
> that FS modules may consume some, but not all, of the public FS API.
>
> I am always a bit nervous about "some, but not all" contracts,
> especially when there is no simple way to spell out what's in the
> "some." So I had actually been assuming that libsvn_fs_subr would
> contain only internal routines, and libsvn_fs (the loader library) would
> wrap around them.
That would be better, yes.
I'll revise the patch to add some trivial wrappers to libsvn_fs so that
clients of the fs api need not link to libsvn_fs_subr.
> When I did the symbol renaming in libsvn_fs_base, I
> went to a bunch of effort to make sure that libsvn_fs_base did not
> consume any of the svn_fs API except for the ID routines; a similar
> amount of effort might still be required for libsvn_fs_fs.
Here is some trimmed objdump output:
DLL name: cygsvn_fs_subr-1-0.dll
svn_fs__canonicalize_abspath
svn_fs__create_id
svn_fs__id_copy
svn_fs__id_copy_id
svn_fs__id_eq
svn_fs__id_node_id
svn_fs_change_txn_prop
svn_fs_check_path
svn_fs_check_related
svn_fs_compare_ids
svn_fs_file_contents
svn_fs_is_file
svn_fs_is_revision_root
svn_fs_is_txn_root
svn_fs_node_id
svn_fs_open_txn
svn_fs_parse_id
svn_fs_purge_txn
svn_fs_revision_root
svn_fs_revision_root_revision
svn_fs_root_fs
svn_fs_txn_root_name
svn_fs_unparse_id
svn_fs_youngest_rev
Max.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sat, 2004-05-01 at 18:17, Max Bowsher wrote:
> Attached is a first draft patch, although I've called it libsvn_fs_subr.
Hm. An important thing to call out about your patch is that
libsvn_fs_subr implements some, but not all, of the FS API, in order
that FS modules may consume some, but not all, of the public FS API.
I am always a bit nervous about "some, but not all" contracts,
especially when there is no simple way to spell out what's in the
"some." So I had actually been assuming that libsvn_fs_subr would
contain only internal routines, and libsvn_fs (the loader library) would
wrap around them. When I did the symbol renaming in libsvn_fs_base, I
went to a bunch of effort to make sure that libsvn_fs_base did not
consume any of the svn_fs API except for the ID routines; a similar
amount of effort might still be required for libsvn_fs_fs.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Max Bowsher <ma...@ukf.net>.
Greg Hudson wrote:
> When I wrote the FS abstraction code, I deliberately introduced some
> circular dependencies between the FS modules and the loader library,
> figuring that it would work out as long as the loader library
> consisted of only one source file.
>
> Well, that castle has apparently come crumbling down; reportedly,
> Windows doesn't allow shared libraries to have undefined symbols, so
> you can only have circular references with a static build.
>
> The circular dependencies consist of:
>
> * The node-rev ID functions, some part of the svn_fs API and some
> internal, which (as previously mentioned) are implemented in the
> loader library.
>
> * A utility function svn_fs__canonicalize_path, which I happened to
> notice when making a pass over libsvn_fs_base (because it was in
> fs.c, which I devoted more attention to). It's not a tremendous
> amount of code, but it's possible that there are other
> FS-independent internal utility functions which could be factored
> out.
>
> Here are possible solutions:
>
> * Introduce a fourth library, libsvn_fs_util, containing the
> node-rev ID functions and svn_fs__canonicalize_path.
>
> * Stuff these functions into libsvn_subr, using internal names.
> (Same idea as the previous one, trading off scope elegance against
> the overhead of creating a new small library.)
>
> * Move svn_fs__canonicalize_path back into the individual filesystem
> modules and work harder at vtable-izing the node-rev ID functions,
> so that there is no need for circular dependencies. I made two
> proposals on how to do this in
> http://www.contactor.se/~dast/svn/archive-2004-04/1461.shtml,
> although none of them are terribly attractive.
>
> * When loading the FS module, pass a pointer to a vtable containing
> the node-rev ID and utility functions located in the loader
> library.
>
> * Move svn_fs__canonicalize_path back into the FS modules, and
> simply duplicate the node-rev ID functions (using internal names)
> in each FS module. When svn 2.0 rolls around and we can
> vtable-ize the node-rev ID functions the easy way, we can
> eliminate the loader library implementation of those functions.
>
> Preferences? I think I side with the last option at the moment, but
> my opinion is fluid.
I favour libsvn_fs_util, mainly because I'd already started in that
direction before Greg's email, but also because duplicating the code in each
fs module seems a little messy.
Attached is a first draft patch, although I've called it libsvn_fs_subr.
Also note that the initialization functions should take an ABI version, like
the ra modules do, but that's a minor point, and I'm out of time for
tonight.
Max.
Re: FS abstraction and circular dependencies
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sat, 2004-05-01 at 21:29, Chris Foote wrote:
> I don't have any problems compiling the FS modules as dlls
> on Windows after simply doing the following:
> 1) remove the defines in svn_private_config.hw to link in the modules.
> #undef SVN_LIBSVN_FS_LINKS_FS_BASE
> #undef SVN_LIBSVN_FS_LINKS_FS_FS
> 2) export the vtables from the FS modules.
> 3) link the FS modules with the FS abstraction.
> 4) rename the FS module dlls to *.so.0 so that they can be loaded.
At least under Unix, we like to give people the option of using normal
dynamic linking for the RA and FS modules, in addition to run-time
DSO-loading. I assume the same is true of Windows. So we can't break
the cycle at that point.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Chris Foote <cf...@v21.me.uk>.
From: "Greg Hudson" <gh...@MIT.EDU>
> When I wrote the FS abstraction code, I deliberately introduced some
> circular dependencies between the FS modules and the loader library,
> figuring that it would work out as long as the loader library
> consisted of only one source file.
>
> Well, that castle has apparently come crumbling down; reportedly,
> Windows doesn't allow shared libraries to have undefined symbols, so
> you can only have circular references with a static build.
>
[snip]
Aren't these symbols in the FS abstraction the problem?
* extern fs_library_vtable_t svn_fs_base__vtable;
* extern fs_library_vtable_t svn_fs_fs__vtable;
as they are in the FS modules, and hence cause the circular dependency.
Once these are removed there is no circular dependency problem for
building shared libraries on windows.
I don't have any problems compiling the FS modules as dlls
on Windows after simply doing the following:
1) remove the defines in svn_private_config.hw to link in the modules.
#undef SVN_LIBSVN_FS_LINKS_FS_BASE
#undef SVN_LIBSVN_FS_LINKS_FS_FS
2) export the vtables from the FS modules.
3) link the FS modules with the FS abstraction.
4) rename the FS module dlls to *.so.0 so that they can be loaded.
The resulting svnadmin dynamically loads the FS module and successfully
creates a new repo.
Although I would suggest doing the following in the fs-loader:
#ifdef SVN_LIBSVN_FS_LINKS_FS_BASE
extern fs_library_vtable_t svn_fs_base__vtable;
#endif
#ifdef SVN_LIBSVN_FS_LINKS_FS_FS
extern fs_library_vtable_t svn_fs_fs__vtable;
#endif
Regards
Chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Greg Hudson <gh...@MIT.EDU>.
On Sun, 2004-05-02 at 09:13, C. Michael Pilato wrote:
> Doh! I was thinking of svn_fs_parse_id(). Sorry about that. But I
> suppose we keep that public for completeness' sake.
Apologies for the confusion. svn_fs_parse_id() is the problematic one
(because it creates an ID from a string, with no FS object to mediate).
It appears to be used only in changes-test.c and in the FS modules
themselves.
(Branko's comment was almost certainly meant to apply to parsing IDs, so
that they can be stored in the working copy, but as he pointed out, his
comment isn't necessarily all that relevant since we can add the APIs we
need at such a time as we start doing that.)
We can't nuke svn_fs_parse_id from the API in 1.x, but we can certainly
worry less about its quality of implementation if we think nobody is
going to use it. So, for now, I will plan to make it so that
svn_fs_parse_id() is deprecated and works only with the base FS
library. If anyone asks for that functionality (I will ask clkao if he
uses it), we will add an svn_fs_parse_id2() which takes an FS object and
works with any FS type.
It also sounds like consensus centers around a libsvn_fs_subr library.
Max appears to be working on that.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Greg Hudson <gh...@MIT.EDU>.
On Mon, 2004-05-10 at 06:01, Greg Stein wrote:
> "could be" is very different from "probably". I'm with Branko here: I
> don't think any of the other forseeable backends would use skels. Thus,
> I'd say to leave them with the BDB implementation unless/until another
> backend needs them. At that point, they can migrate.
Agreed.
> btw, I don't like the library explosion (it becomes very slow to load and
> link all those libs -- just ask the GNOME folks), but libsvn_fs_subr does
> seem to be the cleanest approach at the moment.
For the record, when I did the work, I determined that FSFS really wants
a different ID implementation from BDB. (It's possible to use the same
implementation for both and layer some goo on top in the FSFS layer, but
it's inelegant, and there's just not enough code reuse potential to
justify a sacrifice of elegance.) So, that leaves, for libsvn_fs_subr:
* The key-gen functions
* svn_fs__canonicalize_path
Although I won't object if someone else wants to create a
libsvn_fs_subr, I couldn't justify it to myself for such a small amount
of code. Since only svn_fs__canonicalize_path had been abstracted out
before, I just duplicated it in the two FS modules.
Incidentally, both of those pieces of functionality seem like reasonable
fodder for libsvn_subr (unlike node-IDs), although we'd need to look
carefully for FS-specific assumptions in their contracts.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Greg Stein <gs...@lyra.org>.
On Sun, May 02, 2004 at 08:13:13AM -0500, C. Michael Pilato wrote:
> Branko ??ibej <br...@xbc.nu> writes:
>
> > >Personally, I like the idea of a util library. Candidate contents:
> > >
> > > - the node-rev functions
> > > - svn_fs__canonicalize_path()
> > > - the current contents of libsvn_fs_base/util (skel stuffs)
> > >
> > Skels are totally BDB-specific. I don't think any other database
> > backend would want to use them in favour of a rasonably structured
> > schema.
>
> Skels are not BDB-specific (and yes, I realize that you know this, and
> what you meant when you said the opposite). They could be used by any
> backend that so desires. In fact, they could be used by a different
> BDB implementation if someone is interested in going that route (say,
> the SVN 2.0 schema).
"could be" is very different from "probably". I'm with Branko here: I
don't think any of the other forseeable backends would use skels. Thus,
I'd say to leave them with the BDB implementation unless/until another
backend needs them. At that point, they can migrate.
btw, I don't like the library explosion (it becomes very slow to load and
link all those libs -- just ask the GNOME folks), but libsvn_fs_subr does
seem to be the cleanest approach at the moment.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by "C. Michael Pilato" <cm...@collab.net>.
Branko Čibej <br...@xbc.nu> writes:
> >Personally, I like the idea of a util library. Candidate contents:
> >
> > - the node-rev functions
> > - svn_fs__canonicalize_path()
> > - the current contents of libsvn_fs_base/util (skel stuffs)
> >
> Skels are totally BDB-specific. I don't think any other database
> backend would want to use them in favour of a rasonably structured
> schema.
Skels are not BDB-specific (and yes, I realize that you know this, and
what you meant when you said the opposite). They could be used by any
backend that so desires. In fact, they could be used by a different
BDB implementation if someone is interested in going that route (say,
the SVN 2.0 schema).
The point is that if there is utility code that could be useful to
various backends, it can go to the utility library.
> > - key-gen.[ch] from libsvn_fs_base
> > - ??
> >
> >Also, do we even *need* svn_fs_unparse_id() ?
> >
> Apart from its being used by svnlook?
Doh! I was thinking of svn_fs_parse_id(). Sorry about that. But I
suppose we keep that public for completeness' sake.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by Branko Čibej <br...@xbc.nu>.
C. Michael Pilato wrote:
>Greg Hudson <gh...@MIT.EDU> writes:
>
>
>
>>Preferences? I think I side with the last option at the moment, but
>>my opinion is fluid.
>>
>>
>
>Personally, I like the idea of a util library. Candidate contents:
>
> - the node-rev functions
> - svn_fs__canonicalize_path()
> - the current contents of libsvn_fs_base/util (skel stuffs)
>
>
Skels are totally BDB-specific. I don't think any other database backend
would want to use them in favour of a rasonably structured schema.
> - key-gen.[ch] from libsvn_fs_base
> - ??
>
>Also, do we even *need* svn_fs_unparse_id() ?
>
>
Apart from its being used by svnlook? Not really, but IMHO in the future
we'll have to expose the node ID as a public feature of the FS model in
order to efficiently implement (at least) atomic rename in the WC. That
doesn't mean we'll use this particualr function, though.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: FS abstraction and circular dependencies
Posted by "C. Michael Pilato" <cm...@collab.net>.
Greg Hudson <gh...@MIT.EDU> writes:
> Preferences? I think I side with the last option at the moment, but
> my opinion is fluid.
Personally, I like the idea of a util library. Candidate contents:
- the node-rev functions
- svn_fs__canonicalize_path()
- the current contents of libsvn_fs_base/util (skel stuffs)
- key-gen.[ch] from libsvn_fs_base
- ??
Also, do we even *need* svn_fs_unparse_id() ?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org