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

Meson #170

Closed
wants to merge 7 commits into from
Closed

Meson #170

wants to merge 7 commits into from

Conversation

Salamandar
Copy link

I continued the work of @QuLogic , refactored a bit.

@jplloyd
Copy link
Member

jplloyd commented Feb 25, 2020

Switching the dist to xenial or bionic should fix the meson dependency, though the xenial one is probably too old.

@Salamandar
Copy link
Author

@jplloyd unfortunately Travis only supports up to Bionic, and Meson 0.49.0 is only provided since Disco :'(
https://packages.ubuntu.com/search?keywords=meson

@jplloyd
Copy link
Member

jplloyd commented Feb 26, 2020

@jplloyd unfortunately Travis only supports up to Bionic, and Meson 0.49.0 is only provided since Disco :'(
https://packages.ubuntu.com/search?keywords=meson

If meson >= 0.49 is essential, we can run the meson build on a docker image instead.

Here's an example of a travis configuration using docker for the actual build: https://github.com/mypaint/mypaint-appimage/blob/master/.travis.yml

@Salamandar
Copy link
Author

I'll try that.

@Salamandar
Copy link
Author

OK, I think I had a hold of it. I'll squash the commits to have a clean history.

@jplloyd
Copy link
Member

jplloyd commented Mar 2, 2020

Since the commit history will have to be cleaned up before merge anyway, I suggest you add [skip appveyor] to any further commit messages (first line), as there are currently 4 identical appveyor builds queued up.

@Salamandar
Copy link
Author

Yeah, travis succeedes, I just need to add doxygen inside the docker :)

@Salamandar
Copy link
Author

Weirdly enough, installing sphinx requires timezone configuration… So I removed the docs test.

@jplloyd
Copy link
Member

jplloyd commented Mar 3, 2020

... and when you're only changing the appveyor setup you can add [skip travis] (no underscores) to the first line of the commit message (I was a bit unclear last time - as long as it's on the first line travis/appveyor will respect it).

@Salamandar
Copy link
Author

The Gegl test on Window fails a weird way : it does not find internal Gegl libraries. Maybe that's why gegl was not installed on appveyor ?

@Salamandar
Copy link
Author

Weird, I fixed the Travis build, why did it not run again ?

@jplloyd
Copy link
Member

jplloyd commented Mar 4, 2020

I'll review the changes on Saturday and then we can work on speeding up the CI.

Copy link
Member

@jplloyd jplloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undeniably a lot more readable than the autotools setup and seems to cover everything the current setup does. Can't say that I like the level of magic in the translation build file, but I guess it does the right thing (not that I like the current level of automake magic).
The autotools setup will stick around for a while, so there's plenty of time to fix any further minor (or major) issues.

I've made a couple of comments requesting some details, but other than that;
squash the last commit, drop the shebang commit (unless there's a good reason for it) and I'll merge it.

Comment on lines +46 to +50
&& ninja -C _build test
&& ninja -C _build dist
&& meson /sources _build_gegl --buildtype=release -Dgegl=true
&& ninja -C _build_gegl test
&& ninja -C _build_gegl dist
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming ninja -C _build_gegl test runs a superset of the tests run in ninja -C _build test (it should), we can skip the latter.

@@ -1,4 +1,4 @@
#!/usr/bin/env python
#!/usr/bin/env python3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why make this more constrained, when the script can run on either 2.7 or 3.5+ (and probably more)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC python does not exist on the test machine, or Meson does not find it. I'll try to recall the precise reason.

configuration: conf
)

# TODO change generate.py
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO without any detail of the change = change should be made before merge

- sudo docker pull ubuntu:disco
- sudo docker run -t -v $(pwd):/sources ubuntu:disco /bin/bash -ec "
apt update -y
&& apt install -y meson gettext libjson-c-dev libgirepository1.0-dev libgegl-dev git
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where in the build is git actually used, and why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meson relies on Git to do ninja dist, to "recopy" the source code while preventing recopying any untracked change. It tests the commit, not the source tree.

@nielsdg
Copy link

nielsdg commented Nov 24, 2021

Any possibility of this making any progress @jplloyd or @Salamandar ? :)

@jtojnar jtojnar mentioned this pull request Feb 3, 2024
4 tasks
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants