Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update desktop build to pull from this repo #922

Merged
merged 10 commits into from
Aug 29, 2024

Conversation

kevinschaul
Copy link
Collaborator

Previously the desktop build lived in a separate repo and had to download a released version of the maputnik editor source code. Now that both live in the same repo, the desktop version can simply run the maputnik build command and use those generated files.

This commit also removes the ci-desktop workflow, which is not needed. The regular ci workflow already built the desktop version (this commit also fixes that build).

Fixes #919

If this works for you all, it would be lovely to create a new tag or release on GitHub for two reasons:

  1. So the latest binaries are easier to locate, and
  2. So I can update my submission to homebrew to make installation easier (for os x users at least)

Previously the desktop build lived in a separate repo and had to
download a released version of the maputnik editor source code. Now that
both live in the same repo, the desktop version can simply run the
maputnik build command and use those generated files.

This commit also removes the ci-desktop workflow, which is not needed.
The regular ci workflow already built the desktop version (this commit
also fixes that build).

Fixes maplibre#919
@HarelM
Copy link
Collaborator

HarelM commented Aug 26, 2024

Thanks!!
Any change you can take a look if there are some CI github actions that nees to be updated? I see the following warnings:
image

Have you tested the attached binaries to see that they work as expected?

In regard to tagging, I don't have strong objections, but now it's extremely nice that everything is pushed right to production without extra steps - proper CI/CD style.

How did you plan to decide which tag to put?

In maplibre-gl-js I added a version change check to the npm version and every commit that changes the version and pushed to main will create a tag and a new release.
I've done so in other maplibre libraries as well, see below.
This updates some part of the changelog and opens a PR to bump the version:
https://github.com/maplibre/maplibre-gl-js/blob/bc70bc559cea5c987fa1b79fd44766cef68bbe28/.github/workflows/create-bumb-version-pr.yml
This one will check for a version change and take care of the rest:
https://github.com/maplibre/maplibre-gl-js/blob/bc70bc559cea5c987fa1b79fd44766cef68bbe28/.github/workflows/release.yml

Generally speaking, I'm good with what you decide as long as it doesn't create maintenance overhead (especially the changelog part which is still not automatic for maplibre-gl-js and it probably can be).

Also, it would be great to add a docker image too that is published to ghcr.

@kevinschaul
Copy link
Collaborator Author

Thank you! I've corrected two of those warnings. The others are related to codecov/codecov-action@v3, which looks potentially tricky to update.

I have tested the darwin and linux binaries. Unfortunately I don't have a Windows machine to test that version.

Re: tags/releases, you raise good questions that I'm not sure I'm qualified to answer 🥲. I like what you have done in maplibre-gl-js. It makes sense to me to follow that pattern. Do you want me to try implementing that for this repo?

@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

Sure, it would be great to have an auto generated changelog as opposed to maplibre-gl.
But if that's too complicated then let's keep it for later.
I think we can continue to publish the latest version to docker and the website (github pages) and only publish a desktop version when the version changes.
What do you think?

@kevinschaul
Copy link
Collaborator Author

If you're ok with us doing a manual desktop publish every once in a while, that seems good to me.

@kevinschaul
Copy link
Collaborator Author

One last idea I'll throw out there. We could switch to dated version numbers. Any push to main would updated the version number to the current date. And any push to main would push a release. That way it's all automated and the latest desktop version will always match the version hosted at maplibre.org.

I'm happy with whatever, though!

@HarelM
Copy link
Collaborator

HarelM commented Aug 27, 2024

The date is a nice idea but then you'd need to either commit to main after every commit, not have the version in the package.json or have a non semver version.
Considering the above, manual release of desktop is the lesser evil.
Let me know if you need any help understanding the above gihub yaml files.

@kevinschaul
Copy link
Collaborator Author

Got it. I should have these working tomorrow.

@kevinschaul
Copy link
Collaborator Author

OK @HarelM these workflows are in. I'm not sure how to test besides by trying to make a release. Let me know how you want to proceed! And thanks for all your help.

@HarelM
Copy link
Collaborator

HarelM commented Aug 29, 2024

Let's try it out.
2.1.0 version?

@HarelM HarelM merged commit 66c5a5c into maplibre:main Aug 29, 2024
7 checks passed
@HarelM
Copy link
Collaborator

HarelM commented Aug 29, 2024

HarelM pushed a commit that referenced this pull request Aug 29, 2024
Hopefully will fix CI issue merged in #922

cc @HarelM
@HarelM
Copy link
Collaborator

HarelM commented Aug 29, 2024

Here's the run results,
Seemed to fail on the release note script:
https://github.com/maplibre/maputnik/actions/runs/10619718712/job/29437920164

Also isn't not clear why we are building both web and desktop and only using the desktop output...?

@kevinschaul
Copy link
Collaborator Author

Fixed that in #926.

The web output ends up as an artifact as well, called dist.zip. Happy to remove if you prefer this to only publish the desktop.zip artifact.

@HarelM
Copy link
Collaborator

HarelM commented Aug 29, 2024

We can publish to npm if someone is interested later on, I don't see a real benefit to it as is.
Feel free to remove, although not a critical issue.

@kevinschaul
Copy link
Collaborator Author

OK yep makes sense, I'll remove

@kevinschaul kevinschaul deleted the desktop-ci-cleanup branch August 29, 2024 19:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compile desktop with the current code
2 participants