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

glib-networking: clean up & add installed tests #67957

Merged
merged 11 commits into from Sep 3, 2019

Conversation

@jtojnar
Copy link
Contributor

jtojnar commented Sep 2, 2019

  • built glib-networking
  • ran glib-networking.tests
  • checked that nothing relies on the dependencies formerly propagated (nix-review?)
jtojnar added 5 commits Sep 2, 2019
auto_features are now enabled by default.
This was done from the beginning for no apparent reason:
cc6ecae
So that this can be loaded from programs not depending on gsettings-desktop-schemas.

Currently, this patch is much uglier than it could be due to
https://gitlab.gnome.org/GNOME/glib/issues/1884
@jtojnar jtojnar force-pushed the jtojnar:glib-networking-cleanup branch from 60dbad9 to d531045 Sep 3, 2019
@jtojnar jtojnar mentioned this pull request Sep 3, 2019
6 of 6 tasks complete
find "$installedTests/libexec" -type f -executable -print0 \
| while IFS= read -r -d "" file; do
echo "Wrapping program '$file'"
wrapProgram "$file" --prefix GIO_EXTRA_MODULES : "$out/lib/gio/modules"

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 3, 2019

Member

Why just the gio modules? Couldn't we use wrapGAppsHook, disable it, and wrap them with its args?

This comment has been minimized.

Copy link
@jtojnar

jtojnar Sep 3, 2019

Author Contributor

I do not want to introduce dependencies on GTK, librsvg etc.

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 3, 2019

Member

Right, can we make note of that so someone doesn't refactor it to this down the line?

This comment has been minimized.

Copy link
@jtojnar

jtojnar Sep 3, 2019

Author Contributor

Can we fix the assumptions about wrapGAppsHook instead? It even has apps in the name.

I really need to finish #43150

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 3, 2019

Member

I really need to finish #43150

I do have notes locally on some things I wanted to add to #43150 but documentation writing escapes me also.
Though it would be good, just even as a start, to merge the parts about wrapGAppsHook which I believe was your original intent with this.

Can we fix the assumptions about wrapGAppsHook instead? It even has apps in the name.

When you say "it has apps in the name", it's confusing when it would be used for executables in a library?

This comment has been minimized.

Copy link
@jtojnar

jtojnar Sep 3, 2019

Author Contributor

I do have notes locally on some things I wanted to add to #43150 but documentation writing escapes me also.

I guess I will try to work on I today.

Though it would be good, just even as a start, to merge the parts about wrapGAppsHook which I believe was your original intent with this.

Yeah keeping the scope narrow will make it easier to consider it mergeable. It is just bad that the wrapper connects so much stuff.

When you say "it has apps in the name", it's confusing when it would be used for executables in a library?

For me, an app is primarily a graphical program that a person can use to carry out some task. Though there can be console apps for which we would not want to depend on GTK either. I would not consider such helper of library to be an app.

This comment has been minimized.

Copy link
@worldofpeace

worldofpeace Sep 3, 2019

Member

I do have notes locally on some things I wanted to add to #43150 but documentation writing escapes me also.

I guess I will try to work on I today.

Though it would be good, just even as a start, to merge the parts about wrapGAppsHook which I believe was your original intent with this.

Yeah keeping the scope narrow will make it easier to consider it mergeable. It is just bad that the wrapper connects so much stuff.

Great, I'd be happy to follow up on it once it's merged. And our roadmap can include what needs to be documented in the future.

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Sep 3, 2019

Huh, why does libsoup have?

propagatedUserEnvPackages = [ glib-networking.out ];

(in passthru)

@worldofpeace

This comment has been minimized.

Copy link
Member

worldofpeace commented Sep 3, 2019

checked that nothing relies on the dependencies formerly propagated (nix-review?)

Hmm, maybe we can use staging-next for this 😄
Thinking it's low likelihood.

Also, I think glib-networking has the glib-pacrunner. Could that do with any wrapping?

@jtojnar

This comment has been minimized.

Copy link
Contributor Author

jtojnar commented Sep 3, 2019

Huh, why does libsoup have?

propagatedUserEnvPackages = [ glib-networking.out ];

(in passthru)

I suspect that is because we cannot add wrappers to libraries and we wanted to add TLS support to libsoup.

@jtojnar

This comment has been minimized.

Copy link
Contributor Author

jtojnar commented Sep 3, 2019

checked that nothing relies on the dependencies formerly propagated (nix-review?)

Hmm, maybe we can use staging-next for this smile
Thinking it's low likelihood.

Yeah looking through the reverse dependencies there do not seem to be any obvious issues.

@jtojnar

This comment has been minimized.

Copy link
Contributor Author

jtojnar commented Sep 3, 2019

Also, I think glib-networking has the glib-pacrunner. Could that do with any wrapping?

They use g_proxy_resolver_lookup_async so apparently, yes.

jtojnar added 5 commits Sep 2, 2019
We no longer propagate dependencies so the comment is not relevant.
Add GSettings schemas required for GNOME helper.
@jtojnar jtojnar force-pushed the jtojnar:glib-networking-cleanup branch from d531045 to 0aa934a Sep 3, 2019
@ofborg ofborg bot requested a review from worldofpeace Sep 3, 2019
Copy link
Member

worldofpeace left a comment

Builds and tests passes.

@worldofpeace worldofpeace merged commit 42f63ff into NixOS:staging Sep 3, 2019
14 of 16 checks passed
14 of 16 checks passed
glib-networking, libproxy on aarch64-linux
Details
glib-networking, libproxy on x86_64-darwin
Details
Evaluation Performance Report Evaluator Performance Report
Details
glib-networking, libproxy on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release-combined.nix -A tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@jtojnar jtojnar deleted the jtojnar:glib-networking-cleanup branch Sep 3, 2019
@jtojnar

This comment has been minimized.

Copy link
Contributor Author

jtojnar commented Sep 4, 2019

By the way, I used Coccinelle to create a base patch here as well:

spatch --sp-file hardcode-gsettings.cocci --dir proxy/ --in-place

@r1@
expression GSETTINGS_SCHEMA;
expression settings;
@@
- settings = g_settings_new (GSETTINGS_SCHEMA);
+ {
+ GSettingsSchemaSource *schema_source;
+ GSettingsSchema *schema;
+ schema_source = g_settings_schema_source_new_from_directory ("@gds_gsettings_path@",  g_settings_schema_source_get_default (), TRUE, NULL);
+ schema = g_settings_schema_source_lookup (schema_source, GSETTINGS_SCHEMA, FALSE);
+ settings = g_settings_new_full (schema, NULL, NULL);
+ g_settings_schema_source_unref (schema_source);
+ g_settings_schema_unref (schema);
+ }

Due to this GLib bug, I had to use a second patch:

@r2@
expression CHILD_SCHEMA;
expression settings;
expression parent_settings;
@@
- settings = g_settings_get_child (parent_settings, CHILD_SCHEMA);
+ {
+ GSettingsSchemaSource *schema_source;
+ GSettingsSchema *schema;
+ schema_source = g_settings_schema_source_new_from_directory ("@gds_gsettings_path@", g_settings_schema_source_get_default (), TRUE, NULL);
+ schema = g_settings_schema_source_lookup (schema_source, CHILD_SCHEMA, FALSE);
+ settings = g_settings_new_full (schema, NULL, NULL);
+ g_settings_schema_source_unref (schema_source);
+ g_settings_schema_unref (schema);
+ }

and then had to modify the files with sed -i 's/GNOME_PROXY_.*_CHILD_SCHEMA,/GNOME_PROXY_SETTINGS_SCHEMA "." \0/g' hardcode-gsettings.patch because Coccinelle complained about parse error when I tried to build the child schema id directly in the cocci patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.