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

Simplify Package System #1841

Closed
wants to merge 23 commits into from

Conversation

Projects
None yet
2 participants
@stefwalter
Copy link
Contributor

commented Feb 22, 2015

Our current package system has a number of smells and problems associated with it. Before we go stable, I'd like to revisit this and simplify it. This is good to do know that we know lots more use cases.

Problems

  • The checksum system is too complex.
  • Combining resources from various servers into the same document is an anti-pattern and going to cause us problems.
  • We want to include UI from one package into another on the same server, such as package dialogs in the server summary page.
  • Manifests should be cached, and we shouldn't be shy about adding information there.
  • Each javascript file installed on the server has to be loaded individually via AMD, rather than combined in the dist style of requirejs.

Plan

  • Have one checksum per server.
  • Only servers with an identical set of packages installed have the same checksum, and thus share the browser cache.
  • Only support combining resources (javascript, etc.) from one server in the same document.
  • All (non-static) resources from a server have a prefix of either /cockpit/@host or /cockpit/$checksum. Everything else is relative paths underneath that.
  • Generate the component list in cockpit-bridge.
  • Once a user package is present in ~/.local/share/cockpit then all checksums are turned off.

Advantages

  • Simple to understand: Everything is a relative path.
  • No more silly @@template@@ things
  • We can use a standard AMD loader (like requirejs) instead of inventing our own.
  • We can combine AMD javascript in requirejs style in the bridge for efficient loading.
  • We have a single checksum to identify what the user has installed. This is relevant for support, and diagnosis.

Disadvantages

  • Servers with slightly different sets of packages installed will not share browser cache.
  • Having a user package installed means we don't hit the browser cache.

The disadvantages are, in my opinion, corner cases.

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2015

Basic implementation done. Doing testing.

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2015

Only remaining use of template is @@base@@ which expands to $checksum or @host of the host on which an html file of path / is loaded.

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 23, 2015

Tests pass, Cockpit loads.

Still need to remove old code, rebase, squash, migrate tests.

@stefwalter stefwalter changed the title Simplify Package System WIP: Simplify Package System Feb 24, 2015

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from fef00f9 to 0480013 Feb 24, 2015

@stefwalter stefwalter changed the title WIP: Simplify Package System Simplify Package System Feb 24, 2015

@stefwalter stefwalter removed the needswork label Feb 24, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2015

@mvollmer this is ready for review.

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from 0480013 to 0df06cc Feb 24, 2015

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from 0df06cc to 460b38d Feb 24, 2015

@stefwalter stefwalter removed the needswork label Feb 24, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 24, 2015

Passes VERIFY here.

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from d36bcab to 2e919d1 Feb 24, 2015

@stefwalter stefwalter added this to the Protocol Cleanup milestone Feb 24, 2015

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch 4 times, most recently from f274f53 to 04f7955 Feb 24, 2015

@mvollmer

This comment has been minimized.

Copy link
Member

commented Feb 25, 2015

With a symlink ~/.local/share/cockpit -> /usr/share/cockpit I still get the minified versions of files. It used to be that minified versions of files would be ignored in ~/.local/share/cockpit, no?

@@ -2205,7 +2218,7 @@ function full_scope(cockpit, $, po) {

var fmt_re = /\$\{([^}]+)\}|\$([a-zA-Z0-9_]+)/g;
cockpit.format = function format(fmt, args) {
if (arguments.length != 2 || typeof(args) !== "object")
if (arguments.length != 2 || typeof (args) != "object" || args === null)

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

typeof args? typeof doesn't need parens. Not a strong opinion, of course.

This comment has been minimized.

Copy link
@stefwalter

stefwalter Feb 25, 2015

Author Contributor

Done.


package = g_hash_table_lookup (listing, name);
if (package)
goto out;

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

This used to return NULL for a package that was already added earlier.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

(And this is the reason why my old trick of symlinking /usr/share/cockpit into ~/.local/share doesn't switch off minification anymore.)

This comment has been minimized.

Copy link
@stefwalter

stefwalter Feb 25, 2015

Author Contributor

Good catch. Thanks. Fixed.

@mvollmer mvollmer added the needswork label Feb 25, 2015

typedef struct _CockpitPackage CockpitPackage;
typedef struct _CockpitPackages CockpitPackages;

gint cockpit_packages_port;

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

This doesn't seem to be used anywhere. (There is cockpit_bridge_packages_port for the tests.)

This comment has been minimized.

Copy link
@stefwalter

stefwalter Feb 25, 2015

Author Contributor

Removed.

g_signal_connect (packages->web_server, "handle-resource::/checksum",
G_CALLBACK (handle_package_checksum), packages);
g_signal_connect (packages->web_server, "handle-resource::/manifest.js",
G_CALLBACK (handle_package_manifest_js), packages);

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

Is this used?

This comment has been minimized.

Copy link
@stefwalter

stefwalter Feb 25, 2015

Author Contributor

That is for loading package manifests as an AMD module.

g_ascii_strcasecmp (key, "Upgrade") == 0 ||
g_ascii_strcasecmp (key, "Transfer-Encoding") == 0 ||
g_ascii_strcasecmp (key, "Accept-Encoding") == 0)
continue;

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

What's the reason for filtering these out? Only peformance?

This comment has been minimized.

Copy link
@stefwalter

stefwalter Feb 25, 2015

Author Contributor

The http-stream1 doesn't allow these headers.

}

if (!session)
session = lookup_or_open_session_for_host (self, host, NULL, self->creds, FALSE);
session = lookup_or_open_session_for_host (self, host, NULL, self->creds, FALSE);

This comment has been minimized.

Copy link
@mvollmer

mvollmer Feb 25, 2015

Member

This might be reached with host == NULL. That's what you meant with "$unknown automatically loads the resource from localhost", right?

This comment has been minimized.

Copy link
@stefwalter

stefwalter Feb 25, 2015

Author Contributor

Brought in a FIXUP from another branch.

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from 04f7955 to 0fa8564 Feb 25, 2015

@stefwalter stefwalter removed the needswork label Feb 25, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2015

Fixed review issues, and brought in FIXUPS.

The only known issue is that component frames do not load by checksum. This is a temporary regression to be fixed in the next branch.

@mvollmer

This comment has been minimized.

Copy link
Member

commented Feb 25, 2015

Fixed review issues, and brought in FIXUPS.

Looks good!

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from 0fa8564 to 7fd519e Feb 25, 2015

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Feb 25, 2015

Fixed memory leak

stefwalter added some commits Feb 24, 2015

common: Move the web server code into the common directory
So it can also be used in the bridge.
bridge: Split up HTTP responses into blocks
This is so they don't monopolize the message stream.
bridge: Remove unused cockpit_http_stream_open() function
This isn't used and completely broken. HTTP streams require a large
number of arguments in their open message, such as the method, path
etc.
bridge: Keep a reference on a possibly closing HTTP channel
Because the CockpitChannel may close during this function call
hold a reference to the object.
base: Fix flakey 'http keep alive' test
Force it to use the same connection more agressively, and give it
a unique name, so nothing else can poach this connection.
common: Allow passing -1 as the port to CockpitWebServer
This way it'll not listen on any port by default.
common: Add cockpit_webserver_add_socket() function
This allows us to add an already prepared socket for the web server
to listen on. Used to specify custom address, and so on.
common: Add the cockpit_web_response_pop_path() function
This removes one piece from the path that the CockpitWebResponse
is tracking, and returns it to the caller.
bridge: Add new simpler package code to the bridge
 * Have one checksum per server.
 * Only servers with an identical set of packages installed
   have the same checksum, and thus share the browser cache.
 * Once a user package is present in ~/.local/share/cockpit
   then checksum is turned off.
 * Service package resources via HTTP streams
common: Supply default language to Accept-Language parsing function
Instead of automatically parsing a cookie, we pass in a first
value to be added to the front of the returned array.
ws: Use new simpler packages for serving resources from the web service
 * All (non-static) resources from a server have a prefix
   of either /cockpit/@host or /cockpit/$checksum. Everything
   else is relative paths underneath that.
 * Resources are accessed via http streams
bridge: Add packages tests
These are derived from the old test-resource.c and test-package.c files
base: Move cockpit.packages() API away from resource2 channels
We now use the package listing available via HTTP
base: No need for the AMD loader to resolve packages
Everything is a relative link now.
pkg: Migrate rest of resources to the new packages
 * No more variable expanding, except for the entry point HTML.
 * No need to resolve packages, everything is relative links
bridge: Explicitly connect to 127.0.0.1 when testing packages
This helps work around issues with Travis CI, and the containers
that it runs in.
bridge: Rename the manifest.js .json URLs to 'manifests'
This is a better name indicating it is the result of merging all
the manifests on that server.

Add documentation.

@stefwalter stefwalter force-pushed the stefwalter:package-simplify branch from 7fd519e to 30733b1 Feb 25, 2015

@stefwalter stefwalter deleted the stefwalter:package-simplify branch Feb 26, 2015

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.