Update from upstream

[gjn]

This PR updates our version of ol3 to the upstream version https://github.com/openlayers/ol3/commit/be7ee49701b34fc90ceff2cea6c39869f9836cf5 (some commits after official 3.0.0 release). Last update was June 5th, so there are many changes.

Quite a few adaptions were needed to get the build working. See https://github.com/geoadmin/ol3/commit/ed65cb0d5cebaa8fb45a41970622c261ab77674b. We also created an ol.View2D alieas in order to be compatible with existing API implementations.

One remaing problem: the tests are not working because of the missing GeoAdmin global. But I'm not sure if they worked before. @oterral can you verify?

I will create a PR in mf-chsdi3 which will integrate these changes -> see https://github.com/geoadmin/mf-chsdi3/pull/1007

[oterral]

Yep I'm looking at it.

[gjn]

Maybe tests didn't work before as well? I can't say.

There are some drawbacks using the fork approach...it's not easily maintainable...and it breaks the standard ol stuff (hosted ol3 examples don't work, for example), and I assume that the tests face a similar problem. Of course, removing GeoAdmin global might help...

[cedricmoullet]

I personally develop on my Mac and i dont face these issues. Examples are supposed to work. For tests, i never checked but i don t see any reasons why it should fail. We could rediscuss that, but we dont have to forget the advantages of this approach. Our ol3 API has to be an extension, but not a modification of OpenLayers .

Le 11 sept. 2014 à 15:30, Gilbert Jeiziner notifications@github.com a écrit :

Maybe tests didn't work before as well? I can't say.

There are some drawbacks using the fork approach...it's not easily maintainable...and it breaks the standard ol stuff (hosted ol3 examples don't work, for example), and I assume that the tests face a similar problem. Of course, removing GeoAdmin global might help...

— Reply to this email directly or view it on GitHub.

[oterral]

The target ./build-ga.py check is fixed but that break examples ga-static-build and ga-static-debug.

May be we should remove these 2 examples because we have static example in api3 documentation.

[gjn]

Thanks alot, @oterral.

+1 for removing the static examples.

Regarding the broken ol3 examples: I was talking about the 'hosted-examples' target. There, the ol3 examples don't work. Using serve, they work well.

Regarding the approach for this API: I've never seen the fork approach anywhere else. It feels awkward. Usually, when you use or extend an existing API, you're only concerned with what the API exposes. You don't care about its interals (build system, tests, examples, etc). But because we fork, we have to take care of all the ol3 internals. This complicates maintenance considerably. See all the changes that were needed for this update https://github.com/geoadmin/ol3/commit/ed65cb0d5cebaa8fb45a41970622c261ab77674b. Only a small part were actual code changes. Most was ol3 internals that needed adaptions.

IMO, for this API, we would just have the ol3 artefacts (ol.js, ol-debug.js and ol.css) in the project as resource (this assures that we don't change ol3), extend the things we want to (ga.Map would still be an extension to ol.Map) and create new artefacts (ga.js, ga-debug.js and ga.css), then minimizing our code and concating with the ol3 artefacts. Updating would simply consist of replacing the ol3 artefacts and adapting the code, which would be needed rarely. We don't need examples, as they are duplicated in chsdi3 anyway. In fact, everything should be in chsdi3. I just can't understand why the fork approach was chosen.

Of course, this is me speaking after battling with all of this for a good part of today (Good look finding the relevant changes in 250+ commits and 20k+ changes) . Maybe the biggest storm is over now that 3.0.0 is released (I have my doubts :)), but this doesn't mean that the fork and extend approach is conceptually the best.

[cedricmoullet]

ok for removing static examples.

1. One issue was of course that the build system changed on the OL3 side.... it's not supposed to change every two weeks, but it did happen this time.
2. The "fork" approach was decided for following reasons (of what I remember): a) ol3 is 100% part of GeoAdmin API b) use of closure compiler for GeoAdmin API c) ability to use internal capabilities of OL3 in the GeoAdmin API d) usage of the OL3 build system e) development process as in OL3 f) independency with other GeoAdmin projects. With the approach you indicate, c), d), e), f) wouldn't be ensured.
3. Examples are needed to help the developers to implement the GeoAdmin API.
4. We had previously all in mf-chsdi. The main reason why we decided not to go this way is Google Closure.
5. I think that OL3 will be more stable after this first release. We took the risk to work with the master (since we didn't have any other choices), so your today's hard work was probably the costs of this risk.
6. Some code of build.py has been copied to build-ga.py in this branch. Is this really mandatory ? Maybe it's worth discussing this with Eric.
7. I tested on my mac: tests and examples seems to work fine.

Having said that, I'm of course open to another approach. Doing everything in mf-chsdi3 is something to evaluate. I'm simply not sure that we can make something equivalent to what we have now.

On Thu, Sep 11, 2014 at 6:31 PM, Gilbert Jeiziner notifications@github.com wrote:

Thanks alot, @oterral https://github.com/oterral.

+1 for removing the static examples.

Regarding the broken ol3 examples: I was talking about the 'hosted-examples' target. There, the ol3 examples don't work. Using serve, they work well.

Regarding the approach for this API: I've never seen the fork approach anywhere else. It feels awkward. Usually, when you use or extend an existing API, you're only concerned with what the API exposes. You don't care about its interals (build system, tests, examples, etc). But because we fork, we have to take care of all the ol3 internals. This complicates maintenance considerably. See all the changes that were needed for this update ed65cb0 https://github.com/geoadmin/ol3/commit/ed65cb0d5cebaa8fb45a41970622c261ab77674b. Only a small part were actual code changes. Most was ol3 internals that needed adaptions.

IMO, for this API, we would just have the ol3 artefacts (ol.js, ol-debug.js and ol.css) in the project as resource (this assures that we don't change ol3), extend the things we want to (ga.Map would still be an extension to ol.Map) and create new artefacts (ga.js, ga-debug.js and ga.css), then minimizing our code and concating with the ol3 artefacts. Updating would simply consist of replacing the ol3 artefacts and adapting the code, which would be needed rarely. We don't need examples, as they are duplicated in chsdi3 anyway. In fact, everything should be in chsdi3. I just can't understand why the fork approach was chosen.

Of course, this is me speaking after battling with all of this for a good part of today (Good look finding the relevant changes in 250+ commits and 20k+ changes) . Maybe the biggest storm is over now that 3.0.0 is released (I have my doubts :)), but this doesn't mean that the fork and extend approach is conceptually the best.

— Reply to this email directly or view it on GitHub https://github.com/geoadmin/ol3/pull/109#issuecomment-55290691.

Twitter: http://twitter.com/cedricmoullet Linked In: http://www.linkedin.com/in/cedricmoullet

http://twitter.com/cedricmoullet

[gjn]

1. I can understand c), even though we don't use internals in our code (AFAIK) yet. The other things can be achieved with my approach as well; but without the lock-in we have with the current approach.
2. Examples can easily be done with any approach we choose.
3. Yes, would be happy if @elemoine could review this. I doubt we can get rid of this code duplication easily (it's one of the main reasons I'm complaining in the first place). The fact is that ol3 team does not care about our fork and they do create their build system for their needs. Of course, that's prefectly reasonable for them to do! They should care about the API interface foremost.
4. Tests were fixed by @oterral . Hosted examples (as described in the contribution guide) do not work for the ol3 examples in this branch (maybe not even in master). The served examples work.

Question: are you able to do ./build.py (not build-ga.py) build with this branch without error? I was not able to.

I agree that we can expect that such dramatic changes will not come on a regular basis now that 3.0.0 is out, so switching now is probably not the best choice in terms of cost.

[elemoine]

I'm in Portland for FOSS4G, very jetlagged, but I'm tempted to, myself, try explaining why we deciced to use an ol3 "fork" for the API.

I think the main reason is sharing the project infrastructure. Setting up the infrastructure for working with Closure and everything is not easy. By "forking" ol3 for the API we benefit from the build infrastructure already in place for ol3. I know that the build infrastructure has changed and that this causes you problem. But I'm not sure you'd be in a better situation today if a separate build infrastructure had been created for ga3 at that time. You would still be using Plovr today maybe. I don't know.

But now that we/you have more experience with setting up the infrastructure for Closure and now that we have settled on tools (closure-util) to drive the compiler it might be a good time to revisit that choice. So you could possibly create your own project/infrastructure (based on closure-util) and continue compiling ga3 together with ol3. That is exactly what we do for ngeo.

I hope this makes some sense.

[gjn]

@elemoine thanks for the clarification. Maybe we would have never used plovr in the first place :). ngeo is indeed a good example about another approach. I rest my case now.

Could you review the build-ga.py file in this PR and probably suggest how we could avoid duplication of code (if possible)?

[elemoine]

Maybe we would have never used plovr in the first place :)

You would have needed the same version of the Compiler as the one used by ol3 so you would probably have used Plovr as well. Anyway.

Ragarding reviewing this (and ga-build.py) I'd suggest that you merge the PR if it works for you. I'll do a post-merge review when I'm back from FOSS4G.

[gjn]

That's fine with me, thanks alot @elemoine .

@cedricmoullet please coordinate the merging of this PR with @oterral 's appcache PR, as they conflict.

[cedricmoullet]

@elemoine could you spend some time checking the build-ga.py and provide a feedback ?

[elemoine]

On it. Thanks for the reminder.

[elemoine]

Comments

• I think we could change ol3's pake.py script to allow overwriting targets. In this way you would not need to monkey-pach targets's add method. I can make a PR for that if you agree.
• It may be possible to not entirely duplicate the host_examples function, by having a host_examples_ga function that would call ol3's host_examples function. I can try to provide a PR when this is merged.

[gjn]

@elemoine Thanks alot for the review!

1) that would be great! 2) you can provide a PR for this branch...or I can try directly.

[elemoine]

@gjn can you please try the pake.py script that is in my override branch? This branch makes targets "overridable".

Creating an "override" target is done as follows:

@target('build/jsdoc-%(BRANCH)s-timestamp' % vars(variables), 'host-resources',
              SRC, SHADER_SRC, ifind('config/jsdoc/api/template'), override=True)
def jsdoc_BRANCH_timestamp(t):
    t.run('%(JSDOC)s', 'config/jsdoc/api/ga-index.md',
             '-c', 'config/jsdoc/api/ga-conf.json',
             '-d', 'build/hosted/%(BRANCH)s/apidoc')
    t.touch()

or, when using virtual:

virtual('build', 'build/ol.css', 'build/ga.css', 'build/ga.js',
           'build/ga-debug.js','build/layersconfig', 'build/serverconfig', override=True)

[gjn]

Thanks alot Eric, I'll give it a go this week.

[elemoine]

Thanks alot Eric, I'll give it a go this week.

Thanks. I'd just like to be certain that it works for you before creating a PR for it.

[gjn]

@elemoine hmm. I don't see any difference. Adding the override parameter does not change anything.

Maybe I'm not understanding the purpose correctly, but how does the new pake allow for a simplification of our build-ga.py? There's still duplicated code.

[gjn]

btw: @cedricmoullet I think we can merge this and https://github.com/geoadmin/mf-chsdi3/pull/1007

[elemoine]

My patch addresses this review comment:

I think we could change ol3's pake.py script to allow overwriting targets. In this way you would not need to monkey-pach targets's add method. I can make a PR for that if you agree.

So with my patch you should not need to monkey-patch the target's add method. Does it make sense?

[gjn]

Got it. Yes, this allows for the removal of the add patch, and it works well. Sorry, I misunderstood the purpose.

I pushed it, so you can have a look at https://github.com/geoadmin/ol3/blob/7b45badfee51b7ee6352b8f7459ccaf636974523/build-ga.py. Do you see any other optimisation? I'm still bothered by the duplication of alot of code (esp. the examples rule).

[gjn]

Forgot to ping @elemoine

[cedricmoullet]

@procrastinatio could you please test it and update github.io ?

[procrastinatio]

Local examples (minus the static ones) are OK with ./buildout-ga.py server

[oterral]

@gjn @cedricmoullet @procrastinatio could we merge this one ?

[gjn]

This PR is not merged yet because this branch is not the official ol3 used in api3. This branch represents the ol3 used in the mode=preview api [1]

https://github.com/geoadmin/mf-chsdi3/blob/bfe4c2d4f1a4e95c7babb231c2c89325c61413df/chsdi/templates/loader.js#L38

[gjn]

Here is the PR in mf-chsdi3 containing this branch ol3 version. https://github.com/geoadmin/mf-chsdi3/pull/1007

[oterral]

ok I'll update this branch

[oterral]

Updated to https://github.com/openlayers/ol3/commit/afe1467ddf4be79887f1c674ee9683bd6f151884

[oterral]

And rebased

[oterral]

Test here

[oterral]

The rebase was a very bad idea never do that again @myself.

I put the branch in the same state before the rebase, because if a rebase is done, you can't merge the upstream ol3 without conflicts.