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

gnome-documents: 3.30.0 -> 3.30.1 #55611

Closed
wants to merge 3 commits into from

Conversation

@dtzWill
Copy link
Contributor

commented Feb 12, 2019

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace

This comment has been minimized.

Copy link
Member

commented Feb 12, 2019

worldofpeace added some commits Feb 12, 2019

documents_schemadir = join_paths(documents_datadir, 'glib-2.0', 'schemas')

-if not get_option('buildtype').contains('plain')
+if not get_option('no_network')

This comment has been minimized.

Copy link
@jtojnar

jtojnar Feb 12, 2019

Contributor

Maybe we ought to switch to plain buildtype as it is what upstream recommends https://mesonbuild.com/Quick-guide.html#using-meson-as-a-distro-packager

GNOME Documents probably should not rely on this though and use meson subprojects instead: https://mesonbuild.com/Subprojects.html#download-subprojects

This comment has been minimized.

Copy link
@dtzWill

dtzWill Feb 12, 2019

Author Contributor

I think we should do what you think is best ;), sir resident expert. But I can take a look if discussion is helpful, but if you're saying that's how it is I say let's do it :).

This comment has been minimized.

Copy link
@jtojnar

jtojnar Feb 13, 2019

Contributor

The information about what each build type does is pretty scattered and I am not sure which flags we would need to set using plain. We would probably need to define them in the cross file https://mesonbuild.com/Running-Meson.html#environment-variables
@Ericson2314 might be a better person to tackle this

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Feb 13, 2019

Member

The information about what each build type does is pretty scattered and I am not sure which flags we would need to set using plain.

I made this patch exactly because the plain build type was pretty undocumented.
The pr that introduced this added the condition on plain because it was possible that certain environments wouldn't have git?
Not sure if that was even a proper use.

At any rate do we really need total control of the flags used?

GNOME Documents probably should not rely on this though and use meson subprojects instead: https://mesonbuild.com/Subprojects.html#download-subprojects

Indeed, I should submit a patch. Though do we have support for that in nixpkgs?

This comment has been minimized.

Copy link
@jtojnar

jtojnar Feb 13, 2019

Contributor

Hmm, setup hook should probably add nodownload to mesonFlags. But since the dist tarball already includes the submodule, it is probably not downloaded anyway.

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Feb 13, 2019

Member

GNOME Documents probably should not rely on this though and use meson subprojects instead: https://mesonbuild.com/Subprojects.html#download-subprojects

Actually, I don't think that would do what they want, since it would just fetch it from the submodule's commit and not the latest.

perhaps they could do https://github.com/mesonbuild/meson/blob/master/docs/markdown/Wrap-dependency-system-manual.md#wrap-git

This comment has been minimized.

Copy link
@jtojnar

jtojnar Feb 13, 2019

Contributor

Does the command really fetch HEAD? That sounds like a very bad idea.

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Feb 13, 2019

Member

Does the command really fetch HEAD? That sounds like a very bad idea.

No 😄.
git submodule ui changes too much in my head.

Ahh now I found why they did this

https://gitlab.gnome.org/GNOME/gnome-photos/merge_requests/59
My understanding is that it ensures that the contents
of subprojects/foo to match whatever is committed into Git.

Say I have a stale checkout of a project, with a stale submodule
checkout, on some computer.

Now, in the meantime, the submodule was updated and committed
into Git from elsewhere.

When I come back to the stale checkout and do 'git fetch origin
&& git rebase origin/master', and do a build, I want the contents of
submodule/foo to get force-updated to match Gwhat's known to it as part
of the build.

And meson won't update this once it's fetched.

All of this for:

My main objective was to avoid the manual
intervention, so that I don't mistakenly release broken tarballs and such.

No point really arguing that. I think just checking if git exists would be better, i.e buildtype plain doesn't make sense for this.

This comment has been minimized.

Copy link
@jtojnar

jtojnar Feb 13, 2019

Contributor

And meson won't update this once it's fetched.

I think it will with subproject. And it definitely will be noticed during dist.

This comment has been minimized.

Copy link
@jtojnar

jtojnar Apr 5, 2019

Contributor

Opened as #58310

@dtzWill

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2019

Ping-- is this something we'd like to see merged soon, is it blocking on a discussion or solution?
No worries regardless just making sure it's handled accordingly :).

@jtojnar

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

Updated to 3.32 in #57027.

@jtojnar jtojnar closed this Apr 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.