First login?
Lost password?
 
 
 
General mailing list for discussions and development of PeerLibrary and related software.

List archive Help


Re: [PeerLibrary dev] peerlibrary/meteor vs. meteor/meteor


Chronological Thread 
  • From: Mitar < >
  • To:
  • Subject: Re: [PeerLibrary dev] peerlibrary/meteor vs. meteor/meteor
  • Date: Fri, 22 Aug 2014 12:00:29 -0700
Hi!

> I find the process of forking *everything* we use is somewhat
> paranoid, but then again it has never happened to me that a project I
> use got deleted while I was using it. So from that point of view,
> forking doesn't hurt. :)

The whole idea is to have reproducible stuff. So to know exactly which
version and code we are using and have that available always.

> What is a little problematic is the fact that we *use* these forks in
> production.

But we are not using these forks if there are no changes.

> This forces us to keep them updated manually which doesn't happen
> now.

Create a script. ;-) Maybe for now just a script which checks upstream
things and tell us which stable versions changed.

But updating manually is not really a problem. It is a feature. In
production you do not want latest and shines, but those versions which
work. We even freeze (smart.lock file) packages themselves, even those
which we do not have/use a fork we do update manually (with mrt update).
We want to have slower updating process, so that we can test things.
Once somebody has time and wants to update stuff, they can go around and
update, and then test. We do not yet have really any automatic tests, so
one has to check things manually. So this is good then that it is a
manual step. The only issue in my opinion is that mrt update currently
updates only those which we do not use a fork of. We could have a script
which would tell us that there are also stable changes for those which
we use fork of.

> For example peerlibrary/meteor-file lags 5 commits behind upstream

The only difference is that they ported their package to Blaze (it seems
in a non-backwards compatible way):

https://github.com/EventedMind/meteor-file/compare/peerlibrary:master...EventedMind:master

So we cannot yet update to upstream code anyway.

Other our stuff were made a pull request:

https://github.com/EventedMind/meteor-file/pull/11
https://github.com/EventedMind/meteor-file/pull/17

In fact most of pull requests are ours:

https://github.com/EventedMind/meteor-file/pulls?q=is%3Apr+

We are using a fork, because they have not merged one pull request.

> same can be said for all forks I think.

Please check them and let's see.

> Also, the current workflow actually encourages working in the forks
> instead of pushing the changes upstream.

Not true. It encourages you to create a feature branch and make a pull
request, but also it makes everything already prepared and easy to use
in PeerLibrary, so that you can just continue working, while pull
request is in progress to be merged.

> For example, peerlibrary/meteor is 55 commits ahead of meteor/meteor,
> and I for one have no idea how to get the changes upstream.

Most of those commits are merging from upstream into our fork. You can
see differences here:

https://github.com/peerlibrary/meteor/compare/peerlibrary:master...peerlibrary

It is only 114/57 lines of difference. And we made pull requests for
most of them:

https://github.com/meteor/meteor/pull/1664/files

They decided they want a more generic solution, so we are waiting for
that to get in. (content && content.replace is part of that, because we
have support for binary things, we had to fix that as well, because they
do not want our fix, I have not sent this upstream)

https://github.com/meteor/meteor/pull/2231

This was merged upstream.

In deftemplate.js we have our extension which allows overriding
templates. There is a ticket on this topic upstream, I commented there,
linked to our code, other people used it as well, but ticket was closed,
so I didn't do a pull request:

https://github.com/meteor/meteor/pull/146#issuecomment-25314224

This is probably the only thing which will be a problem when we go to
Blaze. We will have to see how to implement this there. We are using
this so that we can override default login/register templates:

https://github.com/peerlibrary/peerlibrary/blob/development/client/templates/accounts.html

async_multi.js fix is a stupid fix so that our PeerDB tests work. I even
created a reproducible test case for them:

https://github.com/meteor/meteor/issues/2012

test-in-browser changes are here:

https://github.com/meteor/meteor/pull/2357

webapp_server.js and bundler.js changes are our hack to make loading
animation appear first, and to even be able to add loading animation on
the static page Meteor renders. Same for page title. Server-side
rendering is being in the roadmap for later, and this is a very hackish
solution, so I do not think they will accept it upstream so I have not
made a pull request.

packages.js is something we do not need anymore in PeerLibrary, but
there is a pending pull request for those changes as well:

https://github.com/meteor/meteor/pull/1353

So your premise that this does not encourage making pull requests was
just invalided. ;-) I hope this made also you feel less worried about
the state of our fork. It is better than it looks.

Also, now you know what changes are. So it will be easier to update.

OK, I just removed the Npm.resolve stuff, so that it is easier to merge.

> We are now running on an actual fork, and it will require work to
> hop back on meteor/meteor.

But there is nothing wrong with it! This is the beauty of GitHub and
git. Distributed development model. There is no need to go back to
meteor/meteor. Why we would? It is so easy to have and maintain the
fork. See, to get to new stable version, when we want, you do:

git checkout master
git pull upstream master
git checkout peerlibrary
git merge master

And we have a stable version of Meteor with our changes. Merge conflicts
can happen, but as you see, we keep our changes small and we are doing
for most of them pull requests, so there is nothing wrong of having few
changes which we are maintaining, while we wait for things to be merged
upstream. And there will always be things waiting to be merged upstream.
Because we want to run stable Meteor version, and changes we are making
upstream go to development version. So we have to have our fork anyway.
Instead of having to create it every time you make a pull request, we
just have a persistent one.

This is the beauty of git. You do not have to have centralized
development anymore. You do not have ask or have permission to push
upstream. Having and maintaining the fork is easy (most of the merging
is done automatically). But you are still incentivized to make pull
request upstream to keep the changes surface small.

Not to mention, all this is even documented:

https://github.com/peerlibrary/peerlibrary/wiki/Code-Internals#meteor-fork

Feel to contribute more. You can use anything from this e-mails as well.
Now the difference is 59/17. I think this is pretty maintainable, don't
you think?

> As a remedy, one could create the forks (and have them synced every 5
> minutes by a cronjob -- I could set this up)

I bet there is some existing web service of way for making sure your
forks as synced with upstream.

> and leave them wherever they are as fallback -- possibly even a
> different GitHub organization.

I don't think we need a separate organization. I think it is good that
things which are related stay together.

> If we need functionality in upstream packages, development goes
> upstream by the regular fork-branch-pullrequest workflow we all know
> and are familar with.

We do that. The only point is, what we do while the pull request is
being in process of merging (which can take months)? We need our own
fork in meantime.

And sometimes there are changes which will not be merged upstream.
Sometimes changes are good enough for us, but not good enough for
upstream, because they are not generic enough. And we might not have
time to develop the whole generic solution.

We did send quite a lot of pull requests upstream, BTW. For example,
just me:

https://github.com/meteor/meteor/pulls?q=is%3Apr+author%3Amitar+

> This keeps the code we depend on safe and makes sure we don't divert
> too much from upstream.

We keep code safe by merging their stable versions to our fork
regularly. But manually, not automatically, because we have to test
things. In most cases updating is without conflicts so it is easy to do.
In those where merging is not automatic, this means two things:
- maybe our change is not needed anymore
- probably existing open pull request has to be updated so that it can
cleanly merged upstream, so we have to resolve merge conflicts anyway
there as well

> What do you think?

I think we could have a script which:
- if we are using a Meteorite package and we do not have a fork in
peerlibrary organization, it complains
- if we are using a Meteorite package and we do not use a fork,
maintains our fork in sync with upstream
- if we are using our fork in smart.json and there are new changes in
master branch of upstream, which we do not yet have in our branch, it
outputs a message (I would not like to update master branch
automatically, because then it is hard to see what are differences
between our branch and master branch we lastly merged in our branch)

I don't think it has to run regularly. I would say that for now we can
run it manually at same irregular intervals we do "mrt update" from time
to time. So when one developer has time to do that and check things a
bit and so on.

We first need better tests to do things more automatic. giavjeko is
doing amazing job with this, to integrate with SauceLabs:
https://github.com/peerlibrary/travis-ci-meteor-packages/pull/1

Then next step is to start writing tinytests and Selenium tests. I have
no idea how the latter is done, so please help. :-)

Just to add one more worry for you. We also maintain quite some Meteor
packages which wrap some other libraries. We also have to regularly
update them to the latest version of those upstream libraries. Like
PDF.js and Scribe. :-) (I wrote in the recent e-mail more about that.)


Mitar

--
http://mitar.tnode.com/
https://twitter.com/mitar_m

Archive powered by MHonArc 2.6.18.

Top of page