Develop a "patch" approach for updates to ohm-website

danrademacher commented 3 years ago

If we look at the Staging branch of ohm-website, we are 1700 commits behind OSM upstream: https://github.com/OpenHistoricalMap/ohm-website

If we look at the files we have changed since we last caught up with upstream, here's what we've got:

% git diff --name-only f2a386c6b591f9ecf7f21ed1bdb2bd5633b1d2df 11a39b8b05d318357d3a3b284aac2664b58ef721
.gitignore
CODE_OF_CONDUCT.md
Dockerfile
Gemfile
Gemfile.lock
README.md
Vendorfile
app/assets/favicons/ohm_logo.ico
app/assets/images/about/osm copy.png
app/assets/images/attribution_example copy.png
app/assets/images/ohm_logo.png
app/assets/images/ohm_logo.svg
app/assets/images/ohm_logo_256.png
app/assets/images/ohm_logo_30.png
app/assets/javascripts/application.js
app/assets/javascripts/embed.js.erb
app/assets/javascripts/id.js
app/assets/javascripts/index.js
app/assets/javascripts/leaflet.layers.js
app/assets/javascripts/leaflet.map.js
app/assets/javascripts/ohm.style.js
app/assets/javascripts/osm.js.erb
app/assets/javascripts/router.js
app/assets/stylesheets/id.css
app/assets/stylesheets/mapbox-gl-all.scss
app/controllers/users_controller.rb
app/helpers/open_graph_helper.rb
app/mailers/notifier.rb
app/views/export/embed.html.erb
app/views/layouts/_head.html.erb
app/views/layouts/_header.html.erb
app/views/layouts/error.html.erb
app/views/site/about.html.erb
app/views/site/help.html.erb
app/views/site/id.html.erb
app/views/users/terms.html.erb
config/environments/production.rb
config/initializers/action_mailer.rb
config/initializers/assets.rb
config/locales/en.yml
docker-compose.yml
ohm-docker.env.example
public/styles/sprite.json
public/styles/sprite.png
public/styles/sprite@2x.json
public/styles/sprite@2x.png
vendor/assets/iD/iD.js
vendor/assets/iD/iD/locales/en.json
vendor/assets/leaflet/leaflet.osm.js

I am not 100% sure all of those changes are still needed -- for example, changes to iD maybe are no longer relevant, since we're running from a separate iD container? And some, like app/assets/javascripts/ohm.style.js would not have upstream conflicts since they don't exist upstream.

the rest, it seems, would take inspection and then migration to some method of doing these as patches? I'll be hones tthat we've never done that at GreenInfo in any project that I can recall, so it's handwaving on my part.

danrademacher commented 3 years ago

Wait is it as simple as: git diff 11a39b8b05d318357d3a3b284aac2664b58ef721 f2a386c6b591f9ecf7f21ed1bdb2bd5633b1d2df > jan2021-ohmwebsite.patch

then the harder part:

Checkout a fresh upstream branch
apply the patch above
see what's broken?

Step 3 is probably where it could be 1 hour or 100 hours...

batpad commented 2 years ago

Bumping this issue.

We are now further behind upstream and this is beginning to hurt. The really big upstream change that we are missing now is support for JSON in the OSM API. Newer versions of most tools now use the new JSON APIs in some way or another: Tasking Manager, iD, etc. This is going to start hitting us more and more in terms of compatibility with OSM tools.

I really like this patch approach @danrademacher . We have spent some time updating the osm-website version in osm-seed to latest upstream, so we have a working Dockerfile for some of the updates to the rails pipeline, things like that.

Just to reiterate your idea here @danrademacher - we should just checkout latest upstream and apply all the OHM specific changes as a patch. This seems like it would be several hours of work, and most of it is frontend / essentially figuring out applying all the same frontend changes to an "updated" version of the osm-website code. I'm not sure how much the frontend code on osm-website has changed, so I am not sure how much work this would be, but this would be my ideal / preferred approach. The patch file that we develop can also then be maintained separately, in theory making future updates to upstream also much easier.

Of course, if the frontend code has changed a lot, this could be a bit of a rabbit hole - I still believe this is the correct approach, even if it means having to re-wire bits of the frontend.

@danrademacher @jeffreyameyer is it possible to bump the priority of this? Happy to help in any way, though most of the heavy lifting here seems to be on @danrademacher and team, since our changes are mostly frontend.

cc @geohacker @Rub21 @willemarcel

batpad commented 2 years ago

Just also on this patch approach: it doesn't necessarily need to involve anything fancy - with that list of files, it seems like one could also manually just go through each file / replace with the OHM forked version / see what breaks, etc. It maybe interesting to think through the approach / tooling to do this merge - we will need to get a bit creative, but I feel like our changes are contained enough and just being able to see "the file as it was on osm-website before we forked", "the file in our fork", "the file in current osm-website" and diffs between the three somewhat nicely, should make this doable. Of course, this is all contingent on osm-website not having completely changed their frontend, but I don't believe that they have.

willemarcel commented 2 years ago

@batpad I saw that Andy Alan added bootstrap to the frontend around 2020. In terms of JS code, I don't believe we have too many changes.

What if, instead of a PATCH file, we always rebase the repository with upstream and squash our changes in order to have all our changes in only one commit on top of upstream?

danrademacher commented 2 years ago

Wille, that sounds promising!

Regarding patches, I'm not sure the thinking in this issue was really all that fleshed out. I am happy to bump this up priority list, and I think there's going to be work here for both DevSeed and GreenInfo

We have this unfinished PR from Sajjad from Nov 2021: https://github.com/OpenHistoricalMap/ohm-website/pull/63, but I think that we would not use.

To take the patch approach, we would run this updated diff command into a patch file:

 git diff 11a39b8b05d318357d3a3b284aac2664b58ef721 356ac0a13739888073804b190c533494f8742f1d > apr2022ohm-website-upstream.patch

The main benefit of the patch approach as I understand it is we are focused on just getting all our code applied to the latest OSM code, not actually trying to preserve the commit history that got us where we are.

That said, I realize I am not quite sure about the precise next steps:

Based on "We have spent some time updating the osm-website version in osm-seed to latest upstream, so we have a working Dockerfile for some of the updates to the rails pipeline, things like that." It sounds like we need some new Dokcer settings or a new image or something to run locally?
Then check out the latest upstream in a branch?
Then apply the patch? https://git-scm.com/docs/git-apply
then see what is broken

I tried these steps using

Checked out current OHM-staging and did:

ohm-website %  git diff 11a39b8b05d318357d3a3b284aac2664b58ef721 356ac0a13739888073804b190c533494f8742f1d > apr2022ohm-website-upstream.patch

Then switched to https://github.com/OpenHistoricalMap/ohm-website/pull/63 which is the same as https://github.com/OpenHistoricalMap/ohm-website/compare/staging...openstreetmap:master

And did

ohm-website % git apply apr2022ohm-website-upstream.patch
apr2022ohm-website-upstream.patch:83: trailing whitespace.
If you feel you have been falsely or unfairly accused of violating this Code of Conduct, you should notify OpenHistoricalMap with a concise description of your grievance. Your grievance will be handled in accordance with our existing governing policies.
apr2022ohm-website-upstream.patch:99: trailing whitespace.
The Citizen Code of Conduct is distributed by [Stumptown Syndicate](http://stumptownsyndicate.org) under a [Creative Commons Attribution-ShareAlike license](http://creativecommons.org/licenses/by-sa/3.0/).
apr2022ohm-website-upstream.patch:141: trailing whitespace.
      libxml2-dev \
apr2022ohm-website-upstream.patch:206: trailing whitespace.
RUN vendorer
apr2022ohm-website-upstream.patch:513: trailing whitespace.

error: patch failed: .gitignore:1
error: .gitignore: patch does not apply
error: Dockerfile: already exists in working directory
error: patch failed: Gemfile:3
error: Gemfile: patch does not apply
error: patch failed: Gemfile.lock:62
error: Gemfile.lock: patch does not apply
error: patch failed: Vendorfile:20
error: Vendorfile: patch does not apply
error: cannot apply binary patch to 'app/assets/favicons/ohm_logo.ico' without full index line
error: app/assets/favicons/ohm_logo.ico: patch does not apply
error: cannot apply binary patch to 'app/assets/images/about/osm copy.png' without full index line
error: app/assets/images/about/osm copy.png: patch does not apply
error: cannot apply binary patch to 'app/assets/images/attribution_example copy.png' without full index line
error: app/assets/images/attribution_example copy.png: patch does not apply
error: cannot apply binary patch to 'app/assets/images/ohm_logo.png' without full index line
error: app/assets/images/ohm_logo.png: patch does not apply
error: cannot apply binary patch to 'app/assets/images/ohm_logo_256.png' without full index line
error: app/assets/images/ohm_logo_256.png: patch does not apply
error: cannot apply binary patch to 'app/assets/images/ohm_logo_30.png' without full index line
error: app/assets/images/ohm_logo_30.png: patch does not apply
error: patch failed: app/assets/javascripts/application.js:6
error: app/assets/javascripts/application.js: patch does not apply
error: patch failed: app/assets/javascripts/embed.js.erb:31
error: app/assets/javascripts/embed.js.erb: patch does not apply
error: patch failed: app/assets/javascripts/id.js:5
error: app/assets/javascripts/id.js: patch does not apply
error: patch failed: app/assets/javascripts/index.js:19
error: app/assets/javascripts/index.js: patch does not apply
error: patch failed: app/assets/javascripts/index/changeset.js:10
error: app/assets/javascripts/index/changeset.js: patch does not apply
error: patch failed: app/assets/javascripts/index/directions.js:392
error: app/assets/javascripts/index/directions.js: patch does not apply
error: patch failed: app/assets/javascripts/index/export.js:63
error: app/assets/javascripts/index/export.js: patch does not apply
error: patch failed: app/assets/javascripts/index/history.js:149
error: app/assets/javascripts/index/history.js: patch does not apply
error: patch failed: app/assets/javascripts/index/new_note.js:107
error: app/assets/javascripts/index/new_note.js: patch does not apply
error: patch failed: app/assets/javascripts/index/note.js:46
error: app/assets/javascripts/index/note.js: patch does not apply
error: patch failed: app/assets/javascripts/index/query.js:345
error: app/assets/javascripts/index/query.js: patch does not apply
error: patch failed: app/assets/javascripts/index/search.js:127
error: app/assets/javascripts/index/search.js: patch does not apply
error: patch failed: app/assets/javascripts/leaflet.layers.js:50
error: app/assets/javascripts/leaflet.layers.js: patch does not apply
error: patch failed: app/assets/javascripts/leaflet.map.js:18
error: app/assets/javascripts/leaflet.map.js: patch does not apply
error: patch failed: app/assets/javascripts/router.js:148
error: app/assets/javascripts/router.js: patch does not apply
error: patch failed: app/assets/stylesheets/common.scss:2774
error: app/assets/stylesheets/common.scss: patch does not apply
error: patch failed: app/assets/stylesheets/id.css:1
error: app/assets/stylesheets/id.css: patch does not apply
error: patch failed: app/controllers/users_controller.rb:234
error: app/controllers/users_controller.rb: patch does not apply
error: app/mailers/notifier.rb: No such file or directory
error: patch failed: app/views/export/embed.html.erb:2
error: app/views/export/embed.html.erb: patch does not apply
error: patch failed: app/views/layouts/_head.html.erb:6
error: app/views/layouts/_head.html.erb: patch does not apply
error: patch failed: app/views/layouts/_header.html.erb:2
error: app/views/layouts/_header.html.erb: patch does not apply
error: patch failed: app/views/layouts/error.html.erb:6
error: app/views/layouts/error.html.erb: patch does not apply
error: patch failed: app/views/site/about.html.erb:5
error: app/views/site/about.html.erb: patch does not apply
error: patch failed: app/views/site/help.html.erb:4
error: app/views/site/help.html.erb: patch does not apply
error: patch failed: app/views/site/id.html.erb:17
error: app/views/site/id.html.erb: patch does not apply
error: patch failed: app/views/users/terms.html.erb:25
error: app/views/users/terms.html.erb: patch does not apply
error: patch failed: config/environments/production.rb:23
error: config/environments/production.rb: patch does not apply
error: patch failed: config/initializers/action_mailer.rb:1
error: config/initializers/action_mailer.rb: patch does not apply
error: patch failed: config/initializers/assets.rb:16
error: config/initializers/assets.rb: patch does not apply
error: patch failed: config/locales/en-GB.yml:272
error: config/locales/en-GB.yml: patch does not apply
error: patch failed: config/locales/en.yml:253
error: config/locales/en.yml: patch does not apply
error: docker-compose.yml: already exists in working directory
error: cannot apply binary patch to 'public/styles/sprite.png' without full index line
error: public/styles/sprite.png: patch does not apply
error: cannot apply binary patch to 'public/styles/sprite@2x.png' without full index line
error: public/styles/sprite@2x.png: patch does not apply
error: patch failed: vendor/assets/iD/iD.js:25793
error: vendor/assets/iD/iD.js: patch does not apply
error: vendor/assets/iD/iD/locales/en.json: No such file or directory

After that, I see no commits, which is expected since per the docs, git patch doesn't actually generate commits. But I also don't see any modified files. The errors above seem to be saying "patch does not apply" even in cases like this where we know we need changes (loading timeline JS):

error: patch failed: app/views/layouts/_header.html.erb:2
error: app/views/layouts/_header.html.erb: patch does not apply

batpad commented 2 years ago

@danrademacher So, what I did:

Created a new branch from ohm-website staging. Merged in upstream changes to that branch, but always resolving git conflicts to take "our" changes:

git merge -Xours upstream/master

Created a PR against staging: https://github.com/OpenHistoricalMap/ohm-website/pull/87

Also created a patch file, with this branch compared to openstreetmap/openstreetmap-website latest with:

git diff upstream/master > ourpatch.txt

ourpatch.txt - this should represent the "delta" between our changes and openstreetmap-website master. There's a bunch of stuff on the Gemfiles and Dockerfile and some other config stuff that I'm pretty sure we should be able to resolve (@Rub21 let's go over these when you have some time).

And then there's a bunch of conflicts in the frontend code, which don't seem to me to be too severe.

It sounds like we need some new Dokcer settings or a new image or something to run locally?

Yes - when we are running against latest upstream, we will need some changes to the Dockerfile to make everything work. What I'd recommend here possibly as a workflow:

@Rub21 we work together to make a Dockerfile build, etc. work with the new osm website upstream code. We let the frontend code be broken and don't worry about that, just get a working build.
Once that's done, @danrademacher - perhaps you folks can look at the frontend conflicts / test the frontend and that it all seems to work?

danrademacher commented 2 years ago

That sounds like a good plan!

To make sure I'm clear -- we wouldn't try to apply the patch in any automated way, right? We would pick through manually and resolve conflicts, favoring our OHM code only where we actually made changes to it.

I wonder, after this, could could start pulling in upstream say several times a year and it would be alight lift each time. I think we have been saying that since we first forked!

batpad commented 2 years ago

To make sure I'm clear -- we wouldn't try to apply the patch in any automated way, right? We would pick through manually and resolve conflicts, favoring our OHM code only where we actually made changes to it.

That's correct. Would just require going through each file, seeing what's changed upstream, and if we need to incorporate any of those changes. From a brief look at the frontend conflicts, it seems to me like not that much has changed upstream and the changes should be mostly okay to manage (still a bunch of manual work).

I wonder, after this, could could start pulling in upstream say several times a year and it would be alight lift each time. I think we have been saying that since we first forked!

It would be a good idea to have some sort of "cron" Github Action that would just create a pull request with the latest upstream changes every 2 weeks or so that we deal with.

Rub21 commented 2 years ago

@batpad @danrademacher , I went through files and made some updates in the ohm-website branch https://github.com/OpenHistoricalMap/ohm-website/tree/merge-osm-website. according to lates the upstream changes, also made updates in ohm-deploy https://github.com/OpenHistoricalMap/ohm-deploy/pull/137 to build the contianer for these changes, Dockerfile works fine for openstreetmap latest version , but there are some issues in ohm-website https://gist.github.com/Rub21/d898480c04822b1fd9b2443856d56744 , all related to the frontend and javascript dependencies, It may take still some time to figure out more about those issues, maybe someone from the frontend could help us on it.

danrademacher commented 2 years ago

Sorry for the slow reply on this! We can look into the front end issues for sure.

I want to be sure we have fully gotten to same local dev stopping point that you have. Here's what I did:

Checked out https://github.com/OpenHistoricalMap/ohm-website/tree/merge-osm-website
Ran docker-compose build and this seemed to work as usual

Ran docker-compose up -- I thought maybe here I would get similar outputs to yours. Instead, I'm getting an error like this:

web_1  | /var/lib/gems/2.7.0/gems/railties-7.0.3/lib/rails/application/configuration.rb:347:in `database_configuration': Cannot load database configuration: (RuntimeError)
web_1  | Could not load database configuration. No such file - ["config/database.yml"]

I see this file that seems similar but slightly different name: https://github.com/OpenHistoricalMap/ohm-website/blob/merge-osm-website/config/docker.database.yml

Full output of docker-compose up is here: https://gist.github.com/danrademacher/58e0f2201a6530d80a94533a709b4f88

@Rub21 I assume you didn't get this error?

I was running in the background this command as usual, which is how we normally connect to database for local dev: kubectl port-forward service/staging-db 5435:5432 -n default

Rub21 commented 2 years ago

@danrademacher , could you pull the changes?, I have added a example env vars for Database, the above issue should gone.

danrademacher commented 2 years ago

@Rub21 I finally got back to this and I am seeing the same error:

Exiting
web_1  | /var/lib/gems/2.7.0/gems/railties-7.0.3/lib/rails/application/configuration.rb:347:in `database_configuration': Cannot load database configuration: (RuntimeError)
web_1  | Could not load database configuration. No such file - ["config/database.yml"]

I wonder if there's some issue of how I am trying to run this locally that/s at the root of the problem.

Here's what I am doing:

In one terminal window, I run kubectl port-forward service/staging-db 5435:5432 -n default and get the result:
```
Forwarding from 127.0.0.1:5435 -> 5432
Forwarding from [::1]:5435 -> 5432
```
From the ohm-website repo, I do docker-compose build, wait till that's done and then docker-compose up

this time, for good measure I also tried docker-compose build --no-cache, followed by up but I got the same result.

Full log is here: https://gist.github.com/danrademacher/f6917444eebe2a51fde72d37f0624fb1

danrademacher commented 2 years ago

@batpad maybe we should find a time to meet up again in person before you leave Oakland and run through this local dev process together to make sure it's working for both of us at least up to the front end issues we need to resolve?

Rub21 commented 2 years ago

@danrademacher , You don't need to run kubectl port-forward , I wrote some instruction to start the locally.: https://github.com/OpenHistoricalMap/ohm-deploy/blob/new_web_version/images/README.md, It will deploy its own database with local credentials: https://github.com/OpenHistoricalMap/ohm-deploy/blob/new_web_version/images/.env.example , we don't need to make a copy of the env.example, the output should look like so : https://gist.github.com/Rub21/9d537cb049bcddd7ced1c6ba84b0cb59

danrademacher commented 2 years ago

OK, I am testing this now., 13 minutes into the cd images/ | docker-compose build We'll see if the removal of kubectl port-forward is actually an improvement here. If it means that every change to website assets locally requires a 30 minute build, instead of our already very slow 10 minute website-only build, then that'll ratchet up our need to get a better way to develop the website locally.

All stuff @batpad and I can go over on Thursday.

danrademacher commented 2 years ago

OK, report back: The build ran for 35 minutes and then produced an error

=> ERROR [29/42] RUN apt-get update && apt-get install -y libapache2-mod-passenger

Full error log just before and then after the error: https://gist.github.com/danrademacher/5acf1e70225f0b5659a106e2c5a907c2

So different errors than I was getting before, after a longer wait, but still not the same clearly-frontend errors that Ruben found at https://gist.github.com/Rub21/d898480c04822b1fd9b2443856d56744

Rub21 commented 2 years ago

@danrademacher , I can prepare a guide to make the container a development environment, basically redirect some container volumes to a ohm-website. it may help to you test easily the changes.

Rub21 commented 2 years ago

@danrademacher @batpad ,I wrote a guide to test the ohm-website in the docker environment,without compiling every change https://github.com/OpenHistoricalMap/ohm-deploy/tree/new_web_version/images#setting-up-ohm-website-for-development-mode

danrademacher commented 2 years ago

Took a while, but I have gotten this to the point of simple application errors in browser console, as has @batpad

@batpad picking up from Slack, I am now up to Uncaught TypeError: L.OSM.CyclOSM is not a constructor but I had to do the following:

Add gem "mapbox-gl-rails" to the GEMFILE, https://github.com/OpenHistoricalMap/ohm-website/commit/52dd7e04e5cd14044f04d207bfd456228b6125e4
Run bundle install in the container -- this should probably be added to the documentation, but I wish I knew why Yarn was getting GL installed for Sanjay but not for me.

The Ruby GEM method is what we were doing before, so seems reasonable to stick with that, at least until we jump to MapLibre.

danrademacher commented 2 years ago

I have moved the app error fixing over into its own issue. We can keep this issue for the "patch" process, or close it ot in favor of new issues if that's preferred.

danrademacher commented 2 years ago

riffing off of #422 it re-occured to me that we have to think more strategically about all our forks:

ohm-website, the one we just did
iD, update already tracked in #395 but needs to be done and is substantial, https://github.com/OpenHistoricalMap/iD -- almost 5000 commits behind
iD now also has https://github.com/OpenHistoricalMap/id-tagging-schema, which we will have to maintain once we update iD
Tasking manager, https://github.com/OpenHistoricalMap/tasking-manager, about 100 commits ahead and behind right now
Overpass turbo, https://github.com/OpenHistoricalMap/overpass-turbo, super minimal changes on either side
TagInfo, maybe only this? https://github.com/OpenHistoricalMap/taginfo-projects, super minimal changes on either side

This list is a bit daunting, but really it boils down to OHM Website and iD being our major pain points for upstream activity, which is good.

Note that Nominatim, https://github.com/OpenHistoricalMap/ohm-nominatim, ~isn't a fork but instead a set of tools to install Nominatim unmodified and point at OHM data. We should keep it that way if at all possible!~ This is deprecated

danrademacher commented 2 years ago

Other things to keep updated that are version pinned in ohm-deploy but not forked:

Nominatim
Overpass API
Taginfo

@Rub21 and @batpad will write up a list of what versions we are actually using here at this point for these items

danrademacher commented 2 years ago

I realize I let this ticket stray a bit from its original purpose: Focusing on a patch approach to ohm-website particularly and not all the service dependencies. I will move those to a new issue.

Back to the patch approach, here's a command to make the diff between current upstream and our latest commit git diff 31eb7cc11a524427ca11491fe1e9c98d7afbd2b8 6a279be9df074ce2d26d9f7ee23a44796e681c10 > 20220816upstream.patch

Not quite sure which should be a and which b, but in this case, upstream is the base and we're diffing from that.

That produces a 500K line diff file: 20220816upstream.patch.zip

that cover 538 files.

Of those, 281 are iD-related, which we would ideally overwrite with our own iD when the Rails vendorfile processes.

96 are locales files, which have meaningful changes in them for EN and EN-GB and eventually other languages. But upstream can also include new translation keys, so these are a bit hard to manage.

Then we have ~160 other files.

geohacker commented 1 year ago

I tried to use a GH action that would create a PR but that fails, among many reasons because of the conflicts. This action is in this branch https://github.com/OpenHistoricalMap/ohm-website/blob/fork-sync/.github/workflows/sync-fork.yml. I think we should next revisit the patch idea and see if that can be combined with a GH action for regular syncing.

danrademacher commented 6 months ago

This idea of patch approach is a bit stale. Closing this out

OpenHistoricalMap / issues

Develop a "patch" approach for updates to ohm-website #198