Reorganization of files to allow distribution of Julia for embedding without uv.h

[waTeim]

Here is a proposal that Fixes #8690 with minimal changes to the interface. It is partially done, but is enough for a source of discussion and/or a short term fix since no embedding can be done currently without uv.h.

Modifications

1. ios.h is split into ios.h and ios_internal.h
2. Inclusion of uv.h is moved to ios_internal.h.
3. uv struct types are left declared as incomplete-opaque but only if uv.h is not included in julia.h
4. ios_t made incomplete-opaque in ios.h
5. julia_internal.h is modified to include ios_internal.h and julia.h in the right order.
6. Julia source files are modified to include julia_internal.h and/or ios_internal.h as necessary.
7. The definition of ios_t moved to ios_internal.h
8. The definition of jl_uv_file_t moved to julia_internal.h
9. The declaration of jl_spawn moved to julia_internal.h
10. The definition of ios_buffmode_t moved to ios_internal.h
11. The declaration of ios_buffmode move to ios_internal.h

Most of the embedding interface as been left as is except for.

1. As mentioned above. All of the uv_ types are opaque
2. Also ios_t is opaque
3. uv_handle_type is exclusively internal.
4. ios_buffmode_t is exclusively internal.
5. jl_spawn is no longer available to embedding because it uses uv_handle_type as a parameter.
6. ios_buffmode is no longer available to embedding because it uses ios_buffmode_t

Limitations

Opaque uv_types

This is ok since all uv-using functions (except 2) take pointers so these type decls can be left incomplete for purposes of providing parameters, however, the embedder can no longer create the uv parameters directly, but must have a factory provided.

missing uv_handle_type

uv_handle_type could also be made opaque, except for 1 problem, it's an enum, not a struct. Unfortunate, but there is only 1 casualty jl_spawn, which is problem, but not an insurmountable one, if the signature changed, or if there were a different function made available for embedding, then the functionality would be preserved.

missing ios_buffmode

Similar to the above for similar reasons, ios_buffmode is not available.

[tkelman]

this should probably not be inside an #ifdef __cplusplus

[waTeim]

definitely... I've updated.

[tkelman]

The changes here are not too dramatic and look reasonable to me, thanks for working on this. It would be a bit easier to review if you could rebase away the merge commits, to cut down on the noise.

[StefanKarpinski]

I.e. do this while on your local copy of this branch:

git fetch
git rebase origin/master

[waTeim]

Ok did that, resolved conflicts, merged fixed, re-merged and pushed. I think I was successful....

[tkelman]

Almost, but not quite. It seems there are only 4 real commits here. I think the step where things went wrong was the merging - you don't want to merge with this branch, you want to force push over it (prefix the branch name with a + when you do git push). Check the log of your local branch carefully after rebasing but before force pushing, to verify that it matches upstream master plus the 4 relevant commits here.

[nolta]

Also, those 4 commits should be squashed into 1.

[waTeim]

heh, sorry this is turning into the help the git-newb,

Yes there are 4 real commits for this. Everything else is my attempt to equate where upstream is except for the stuff I want to change.

The merge was a result of a rebase that had conflicts. I guess there are none now? If I re-rebase will it be more/less successful? I've already rebased though won't it just basically do nothing or replay the conflicts?

But, I can do a push -f no problem.

To squash all commits into 1 commit, I've read that you can rebase a branch into itself. Something like

 git rebase -i HEAD~5

To make sure should I fetch upstream and merge master and then rebase the merged master (origin/master) or should I fetch and merge in the branch and then rebase itself onto itself? Or was @StefanKarpinski 's comment about fetching the branch remote to avoid problems with the local branch and the remote?

[tkelman]

If I re-rebase will it be more/less successful?

I'm not sure, since there appear to be 2 copies of the commits here. Worst case you may have to do the same conflict resolution again, hopefully it wasn't too bad.

Since this branch has some nonlinearity going on, I would personally do git rebase -i SHA where SHA is the newest commit in the log that comes from upstream. Or do git rebase -i origin/master while on this branch, picking and squashing into just a single commit on top of the latest from upstream.

[waTeim]

I'm not sure, since there appear to be 2 copies of the commits here. Worst case you may have to do the >same conflict resolution again, hopefully it wasn't too bad.

Nah, wasn't bad, just 2 files, but it feels like that is what caused the merge, so I'm worried I'll just end up with another merge. OTOH, I'll just end up with the same thing nothing worse.

Do I need to first fetch from upstream, merge into master and then rebase master if I rebase origin/master? Otherwise do I fetch upstream then git rebase -i SHA the most up-to-date commit of the thing I just fetched? Seems the latter is preferable like you said.

[tkelman]

You shouldn't usually need to do merges into the master of your fork, unless it's got some leftover commits that you made some time ago. If you don't care about those any more, you may want to force push over them so your fork is just a delayed copy of upstream's master and you only ever need to do fast-forward pulls or pushes to update your fork's master.

What's your favorite workflow for creating a new branch in your fork to work on a new PR? The branches dropdown of your fork in the github web ui? git checkout -b newbranch on the command line? The edit button from a file in this upstream repo? The state of your fork's master only makes a difference for the first of those.

For this PR which already has a branch created, you shouldn't need to do any merging - one of the collaborators will merge it into upstream master once it's done (likely via the merge button in the web ui), but that should be the only merge commit.

You also probably don't need to fetch either, unless any upstream changes have introduced new conflicts. Here's the process I just walked through:

$ git remote add waTeim https://github.com/waTeim/julia
$ git fetch waTeim > /dev/null
$ git checkout waTeim/waT/uvsplit
$ git checkout -b waT/uvsplit
$ git rebase -i origin/master
# pick first of 4 relevant commits, squash other 3 relevant commits, remove all others
$ git log
commit cb9a318cc403c8c7188426f31c45a957374c6802
Author: waTeim <[email protected]>
Date:   Sat Nov 1 20:05:55 2014 -0400

    Reorganization of headers to prevent the need for exporting libuv
    definitions to programs that embed Julia.

    Femto Lisp include changes required discovered by rebuild.

    changed comment of ios_buffmode to a deletion

    Moved decl of jl_spawn inside of extern "C" {}

commit 27f1b8154873a25492482d7856e775f027dcb0c0
Author: Viral B. Shah <[email protected]>
Date:   Tue Nov 4 00:49:05 2014 +0530

    Fix #8874

$ git push mine waT/uvsplit
Username for 'https://github.com':
Password for 'https://[email protected]':
Counting objects: 719, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (342/342), done.
Writing objects: 100% (719/719), 112.84 KiB | 0 bytes/s, done.
Total 719 (delta 577), reused 508 (delta 369)
To https://github.com/tkelman/julia
 * [new branch]      waT/uvsplit -> waT/uvsplit

result at: https://github.com/tkelman/julia/tree/waT/uvsplit

Edit: in your case, the final push would need to be a force push over this branch, so git push waTeim +waT/uvsplit

[waTeim]

Argh. That merge. WTH?

What's up with Travis?

ld: file not found: /usr/local/lib/gcc/x86_64-apple-darwin13.4.0/4.9.1/libgfortran.3.dylib for architecture x86_64

On branch master I did

git pull upstream master

On branch waT/uvsplit I did

git rebase -i origin/master

There were double the number of commits (2 of the same message) I selected the last ones and squashed the rest and deleted the dups.

I re-verified my embed test still works (it's simplistic). But my other project fails now. Does the type String no longer exist? Is it AbstractString only or something?

[tkelman]

What's up with Travis?

Apparently homebrew upgraded gcc to 4.9.2, so we need to ignore osx travis until new binaries are built.

But my other project fails now. Does the type String no longer exist? Is it AbstractString only or something?

Yeah, #8872

What does git remote -v say? If origin is your fork, you should actually be rebasing against the upstream remote, sorry. I was under the assumption you originally cloned from JuliaLang, not your fork.

[waTeim]

Oh! #8872. Well that shows that as least I'm picking up the new stuff, and I'm glad I know about it now. But how is the same source to be used for .3.x and .4.x in this case String doesn't exist in .4 and AbstractString doesn't exist in .3.x... Well kind of another topic.

What does git remote -v say? If origin is your fork, you should actually be rebasing against the upstream >remote, sorry. I was under the assumption you originally cloned from JuliaLang, not your fork.

LoL, well that explains it. I thought everyone branched from their own master (git checkout -b from master) and then periodically updated their own master from upstream/master. Heh, I guess not, we
see what happens!

bizarro% git remote -v
origin  [email protected]:waTeim/julia.git (fetch)
origin  [email protected]:waTeim/julia.git (push)
upstream    [email protected]:JuliaLang/julia.git (fetch)
upstream    [email protected]:JuliaLang/julia.git (push)

I can rebase again! This time upstream/master, What's likely to happen?

[tkelman]

how is the same source to be used for .3.x and .4.x in this case String doesn't exist in .4 and AbstractString doesn't exist in .3.x... Well kind of another topic.

The right answer for that is to open an issue (or better, a PR) at https://github.com/JuliaLang/Compat.jl actually nevermind, there's a deprecation so String should keep working for a while.

I thought everyone branched from their own master (git checkout -b from master) and then periodically updated their own master from upstream/master.

You can do it that way, but your fork's master has a bunch of accumulated merge commits. As I said earlier, I'd check through the comparison between your fork's master and JuliaLang's master to see whether there's anything there you want to keep. If not, I would checkout upstream/master and force push over your fork's master. From then on your pulls and pushes should all be clean fast-forwards, with no extra merge commits cluttering up the history.

This time upstream/master, What's likely to happen?

Should end up with a commit that looks identical to waTeim@00e25af but with a parent straight from upstream, instead of one of your merge commits.

[waTeim]

So is this a problem?

julia> String
ERROR: String not defined

julia> using Compat

julia> @compat String
ERROR: String not defined

julia> @compat AbstractString
AbstractString

finally. I ended up with

git rebase -i upstream/master

But threw out all but the last thing. The historical stuff was from June, and could not be merged because of submodule differences. So I took a guess and just took the last. The diffs look correct.

[tkelman]

Git drama resolved, thanks for sticking with it.

@vtjnash or @stevengj, can you look over this? Is losing jl_spawn from the embedding API that big of a deal?

[vtjnash]

that function (and the rest of jl_uv.c) was never really intended for embedded use anyways. it shouldn't be considered part of the (semi-)stable Julia API: it's an implementation detail of spawn in base.

[vtjnash]

the loss of ios_t does seem problematic however, since it precludes programs from creating new iostream files

[waTeim]

Maybe a new constructor function?

ios_t *ios_new(args)

What would such a constructor take? Or how about something like

iot_t *ios_new()
{
   ios_t *ios = (ios_t*)malloc(sizeof(ios_t));
   _ios_init(ios);
   return ios;
}

[vtjnash]

would also need an ios_free then, but yes, that's another option. why does ios_t need to get removed?

[waTeim]

Because of the field bm

bufmode_t bm; 

buffmode_t requires uv.h

typedef enum { bm_none=UV_HANDLE_TYPE_MAX+1, bm_line, bm_block, bm_mem } bufmode_t;

[StefanKarpinski]

Git drama resolved

I really like the phrase "git drama".

[stevengj]

Wouldn't it be cleaner to just install our own uv.h in /usr/include/julia?

[waTeim]

Well does the embedding client need any uv_ stuff? If it's the case that none of jl_uv.c stuff was intended for embedding, then maybe all of the uv_ declarations should be moved to julia_internal.h and only have ios_t available opaquely for stream creation.

This is kind of the half-way-not-sure attempt.

uv can also be globally installed, and if the embedding client also uses libuv (semi)-directly then I can say from personal experience there can be problems.

So if there is a global uv.h and a julia uv.h, which is the correct one? Depending on the include order, it will be the one that's not included. Tricky.

So I'd say the cleaner solution is to export only what is absolutely necessary, but of course I'm going to say that.

[stevengj]

It is often useful for C code calling Julia use uv functions. A good example is uv_async_send, which is necessary if you have a thread that needs to talk to Julia asynchronously.

Conflicts shouldn't be a problem: any program embedding Julia will need to link to Julia's libuv anyway (it isn't practical to link to two different versions of libuv simultaneously), so they will need to include julia/uv.h to get the Julia header. Include order is not an issue if you put it in /usr/include/julia and include the julia/ path explicitly in #include.

[waTeim]

I have not had a need to asynchronously call Julia yet, but rather asynchronously call my own thread that synchronously calls Julia, but that sounds interesting. It's not really documented anywhere I've read though. Was that done with jl_spawn or something?

it isn't practical to link to two different versions of libuv simultaneously

This can be done for at least 2 platforms (OS/X and Linux) from what I've seen so far. I agree that to communicate with Julia using uv, you have to use the uv that Julia was built with, but conversely, to communicate with another framework that uses another uv, you have to use that other framework's uv, and uv is popular enough that you can expect that other people will eventually try to use such a scheme. In such a case interacting with the underlying uv would have to be masked somehow. It would be better to avoid forcing embedding to use Julia's notion of uv unless it's necessary. An API that uses uv internally but exports something Julia only seems cleaner because less baggage.

[nalimilan]

I think the long-term goal should be to use the standard libuv, in which case the problem of conflicting version shouldn't arise.

[stevengj]

The use of uv_async_send is documented in the manual.

When you are linking to both Julia (and Julia's libuv) and also an external libuv, and you call a libuv function (e.g. uv_async_send), how does it decide which version to use? Are you linking one of them in an RTLD_LOCAL way or something?

[tkelman]

I think the long-term goal should be to use the standard libuv

We'll have to bump joyent/libuv#451 back to life to make any progress there. libuv's also about to release 1.0.0 and start following semver rigorously, they might be hesitant about a change that would probably force them to go to 2.0.0 so soon.

[vtjnash]

If they don't want to jump to 2.0.0, it would be nice if they would fix their API first, so that we no longer need a fork to make pipes work with process spawn (and other minor issues). That's not really up to us, however.

[waTeim]

long-term goal should be to use the standard libuv, in which case the problem of conflicting version shouldn't arise.

I'm not sure that's workable; won't you have to coordinate with at least Joyent and they're using 3 simultaneously - libuv 0.10.x libuv 0.11.x and libuv 1.0.0 <--- as of Sept 24. Not all in 1 deployment!

When you are linking to both Julia (and Julia's libuv) and also an external libuv, and you call a libuv function (e.g. uv_async_send), how does it decide which version to use?

Heh, now we're getting into #8864, this was revelation that kind of happened by accident when I saw that PPA Ubuntu was working no problem but should not have. But @staticfloat is the one that determined the reason. If the embedding program relies on 2 frameworks A and B use of uv, then on Linux linking with -Bsymbolic-functions will cause A to use uv A, and B to use uv B. OS/X appears to do this by default. The reason we discovered this is Ubuntu uses that flag by default when creating PPA. Apparently, on Windows, this is also possible, but I have no direct experience with it yet.

Oh yea, and also

#if linux
   (void)dlopen(JULIA_LIB "/libjulia.so",RTLD_GLOBAL|RTLD_NOW);
#endif

Hmm, RTLD_GLOBAL versus LOCAL? I'm seem to recall LOCAL not working. I shall re-test. But that was to resolve other link problems, not uv stuff.

What happens if the embedding program uses uv_async_send directly, I'm not sure, but probably the one that's listed first on the command line in the link step, that correlates at least with my experience. But if it uses uv_async_send indirectly (like above) it's going to end up using the uv_async_send of the framework. So, if Julia were to have a function like jl_async_send, simply a convenient wrapper for uv_async_send, then it would resolve to the one that Julia expects.

[tkelman]

This is starting to sound like what I wound up doing for BLAS - we could objcopy/objconv all of our uv_ into jluv_ or something? Not sure we want to though.

[staticfloat]

Regarding the embedded troubles that @waTeim faced, we don't need to do anything so exotic as what we needed to do with ILP64 BLAS to make it play nicely. In this case, we aren't doing runtime lookup of libuv functions through something like ccall, we're linking to them through the system linker so we can use -Bsymbolic-functions to force the linker to resolve as much as it can at libjulia build time, rather than at libjulia dynamic link time (e.g. when it's being opened up by Node)

[nalimilan]

We'll have to bump joyent/libuv#451 back to life to make any progress there. libuv's also about to release 1.0.0 and start following semver rigorously, they might be hesitant about a change that would probably force them to go to 2.0.0 so soon.

@tkelman Sounds like a good idea anyway, better discuss this patch sooner than later. It's too bad they didn't reply earlier...

[waTeim]

It's been a while, are you looking for me to go ahead and take care of ios_new and ios_free? That would be simple. Is more discussion needed?

[ViralBShah]

Do we need to do this still? The temp fix actually seems to be ok to me.

[tkelman]

The temp fix actually seems to be ok to me.

For now. If we can figure out how to get away without installing uv.h down the line it would still probably be a good idea.

[ViralBShah]

My understanding is that the common case doesn't need it. However, there are a few documented cases where we need to have it (asynchronous) - whether provided by us, or having our modifications accepted upstream. It seems that even if we separate out our internals, we do not have a way to get away without installing uv.h completely.

[waTeim]

Exporting functionality that uses uv underneath is all that's required, so long as sufficient factories of equivalent opaque types that refer back to uv internally are available, uv.h would not be necessary outwardly. But If you mean at minimum uv.h is needed to compile Julia. yes.

[vtjnash]

We still could perhaps do this, but this PR is old enough that I'm guessing it probably won't be a helpful starting point anymore.