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

Re-order the sourcemap writing to match spec #2193

Merged
merged 1 commit into from
Oct 22, 2016

Conversation

nschonni
Copy link
Collaborator

The spec
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6f
AH0KY0k/edit?pli=1# outlines the order of the elements as:
Line 1: The entire file is a single JSON object
Line 2: File version (always the first entry in the object) and must be
a positive integer.
Line 3: An optional name of the generated code that this source map is
associated with.
Line 4: An optional source root, useful for relocating source files on
a server or removing repeated values in the “sources” entry. This
value is prepended to the individual entries in the “source” field.
Line 5: A list of original sources used by the “mappings” entry.
Line 6: An optional list of source content, useful when the “source”
can’t be hosted. The contents are listed in the same order as the
sources in line 5. “null” may be used if some original sources should
be retrieved by name.
Line 7: A list of symbol names used by the “mappings” entry.
Line 8: A string with the encoded mapping data.

@xzyfer xzyfer added this to the 3.4 milestone Sep 26, 2016
@xzyfer xzyfer modified the milestones: 3.4.1, 3.4 Oct 4, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Oct 4, 2016

Removing this from the 3.4 milestone. I'm not comfortable fixing what isn't broken for users. Let's see what Ruby Sass does. This may be more appropriate for 4.0.

@saper
Copy link
Member

saper commented Oct 4, 2016

Looks pretty straightforward change to me.

@nschonni
Copy link
Collaborator Author

nschonni commented Oct 4, 2016

Ugh, tried that "Update Branch" button but it ended up adding a merge commit. I'll rebase it properly when I'm home.

@mgreter
Copy link
Contributor

mgreter commented Oct 20, 2016

In terms of json this is a noop. From http://www.rfc-editor.org/rfc/rfc7159.txt:

An object is an unordered collection of zero or more name/value
pairs, where a name is a string and a value is a string, number,
boolean, null, object, or array.

So this should not have any impact on stuff that works according to the json specs. The current srcmap spec draft (SourceMaps V3 is IMO still just a proposal/draft) only mentions the version explicilty to be the first entry (which by itself goes kind of counter the json specs), but AFAIR libsass should adhere to this as IMO our json generator keeps the ordering of keys.

The spec
https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6f
AH0KY0k/edit?pli=1# outlines the order of the elements as:
Line 1: The entire file is a single JSON object
Line 2: File version (always the first entry in the object) and must be
a positive integer.
Line 3: An optional name of the generated code that this source map is
associated with.
Line 4: An optional source root, useful for relocating source files on
a server or removing repeated values in the “sources” entry.  This
value is prepended to the individual entries in the “source” field.
Line 5: A list of original sources used by the “mappings” entry.
Line 6: An optional list of source content, useful when the “source”
can’t be hosted. The contents are listed in the same order as the
sources in line 5. “null” may be used if some original sources should
be retrieved by name.
Line 7: A list of symbol names used by the “mappings” entry.
Line 8: A string with the encoded mapping data.
@nschonni
Copy link
Collaborator Author

Not sure why Appveyor is stalling out in Debug mode.

@mgreter
Copy link
Contributor

mgreter commented Oct 21, 2016

Code is much slower in debug mode (no code optimizations), therefore the tests take too long. I guess the fluctuations are due to the shared resources of the appveyor CI hosts (they probably also tweak the settings from time to time). Normally we sit this out; otherwise we need to disable the test.

@nschonni
Copy link
Collaborator Author

It's odd because the other ones finish in 3 minutes, but it looks like that one timed out after an hour. Can't see whats actually fail though

@mgreter
Copy link
Contributor

mgreter commented Oct 22, 2016

As i said the code is really really really much slower in O0 vs O2 ;) It really just timed out, pretty positive about that.

@mgreter
Copy link
Contributor

mgreter commented Oct 22, 2016

If you really want to check you need to run the debug build against the spec yourself. The only other reason could be some assertion that is triggered, but AFAIR this should no longer halt the CI (I remember that I fixed that some long time ago).

@mgreter
Copy link
Contributor

mgreter commented Oct 22, 2016

Got me thinking and this might be again #1883 🙄

@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

I've noticed this happening recently. I don't think it's related to the build time. The build completes but CI hangs for over 30minutes seemingly waiting to execute sass-spec.

Fetching info for PR 2193
Run options: --seed 42180
# Running:

@mgreter
Copy link
Contributor

mgreter commented Oct 22, 2016

@xzyfer I think we just don't see the dots progressing as the output is probably buffered ...

@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

You may be right.

@mgreter
Copy link
Contributor

mgreter commented Oct 22, 2016

@xzyfer if you want to merge this later anyway, you might as well merge it right now! I have nothing against it, as it doesn't make a difference, and the new order seem reasonable regarding the sepcs-drafs-proposal.

@xzyfer xzyfer merged commit 9476a9a into sass:master Oct 22, 2016
@xzyfer
Copy link
Contributor

xzyfer commented Oct 22, 2016

👍

@mgreter mgreter modified the milestones: 3.4, 3.4.1 Oct 22, 2016
@nschonni nschonni deleted the sourcemap-order branch October 22, 2016 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants