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

Use javascript and CSS .map files for debuginfo #5191

Closed
wants to merge 9 commits into from

Conversation

@stefwalter
Copy link
Contributor

commented Oct 20, 2016

This is a whole bunch of changes which change our debuginfo to use the map files that webpack and other tools generate. This brings them much more inline with the debuginfo we include for binaries.

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from 8323fc7 to 504c6b0 Oct 20, 2016

@stefwalter stefwalter referenced this pull request Oct 20, 2016
3 of 4 tasks complete

@stefwalter stefwalter added blocked and removed needswork labels Oct 20, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Oct 20, 2016

Blocked on #5182

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from 504c6b0 to 4431488 Oct 21, 2016

@stefwalter stefwalter removed the blocked label Oct 21, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor Author

commented Oct 21, 2016

Rebased.

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from 4431488 to 7c849a3 Oct 21, 2016

@stefwalter stefwalter removed the needswork label Oct 21, 2016

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from 7c849a3 to da5af9c Oct 22, 2016

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Oct 22, 2016
test: Install debuginfo when running tests
Now that all the debuginfo is either ELF symbols and GDB data
or javascript and CSS map files, it's not a problem to have it
installed during the tests.

Our PhantomJS driver doesn't yet use this information properly.
That's for a future pull request to work through.

Closes cockpit-project#5191

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from da5af9c to e78a306 Oct 22, 2016

@dperpeet

This comment has been minimized.

Copy link
Member

commented Oct 24, 2016

build seems to fail everywhere

@dperpeet dperpeet added the needswork label Oct 24, 2016

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from e78a306 to 1c17e2e Oct 24, 2016

@stefwalter stefwalter removed the needswork label Oct 24, 2016

stefwalter added 9 commits Oct 20, 2016
Makefile.am: Cleanup the building of min.js and min.css files
This is only done from automake in the base1 package now. So lets
move this logic there and make it less magical.

Also remove unused definitions and rules.
tools: Remove unneeded jsbundle tool
We can just concatenate the few files remaining files that
we need to bundle. They all automatically fill in their own
AMD module id.

Note that the AMD stuff and bundle.js is legacy, and no longer
used by any of our own code.
base1: Ship map files for javascript and CSS in this package
Use cleancss as it generates source maps for CSS files.
Makefile.am: Fix tools/make-rpms script for new node_modules
Due to the way cleancss loads its dependencies, it needs to
be a symlink in the .bin directory. Fix the old style tar
created by the default dist-gzip Makefile target.
test: Install debuginfo when running tests
Now that all the debuginfo is either ELF symbols and GDB data
or javascript and CSS map files, it's not a problem to have it
installed during the tests.

Our PhantomJS driver doesn't yet use this information properly.
That's for a future pull request to work through.
base1: Put tarball distributable stuff in dist/ directory
This makes things more consistent with the other packages, even
though the base1 package is not using webpack.

Other projects also use dist/ as their location for prebuilt
ready stuff.

@stefwalter stefwalter force-pushed the stefwalter:debug-map-files branch from 1c17e2e to e194b3d Oct 25, 2016

@@ -2885,7 +2885,8 @@ table.datatable th:active {
}
@font-face {
font-family: 'PatternFlyIcons-webfont';
src: url('fonts/patternfly.woff') format('woff');
src: url('../fonts/PatternFlyIcons-webfont.eot');
src: url('../fonts/PatternFlyIcons-webfont.eot?#iefix') format('embedded-opentype'), url('../fonts/PatternFlyIcons-webfont.ttf') format('truetype'), url('../fonts/PatternFlyIcons-webfont.woff') format('woff'), url('../fonts/PatternFlyIcons-webfont.svg#PatternFlyIcons-webfont') format('svg');

This comment has been minimized.

Copy link
@dperpeet

dperpeet Oct 26, 2016

Member

as stated on irc: I don't understand why all these urls need to be on the same line

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 26, 2016

Author Contributor

Upstream in patternfly they are on teh same line ... so you might choose to file a bug there.

That said, this pull request should not change any lines in this file. Thanks for catching this.

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 26, 2016

Author Contributor

Oh, duh. I misread the path for this. This is the original patternfly file in /lib being shipped, and git thinks the file has moved. It hasn't. So there is no bug here. If you want the urls not to be on the same line you should file a bug against patternfly upstream itself.

This comment has been minimized.

Copy link
@stefwalter

stefwalter Oct 26, 2016

Author Contributor

I've verified that the patternfly CSS installed and distributed by Cockpit does not have this multi url line. It merely shows up here as the original file, which is amended during the build with our sed script.

This comment has been minimized.

Copy link
@dperpeet

@stefwalter stefwalter added needswork and removed needswork labels Oct 26, 2016

@dperpeet dperpeet self-assigned this Oct 26, 2016

@dperpeet dperpeet closed this in c4ceee4 Oct 26, 2016

@stefwalter stefwalter deleted the stefwalter:debug-map-files branch Oct 31, 2016

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