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

Add a combined source without #line directives into the dist package #363

Merged
merged 5 commits into from Nov 4, 2015

Conversation

Projects
None yet
3 participants
@svaarala
Owner

svaarala commented Sep 17, 2015

Related to #362. Prototype how duktape.c would work without #line directives:

  • Add optparse to combine_src.py
  • Add a --line-directives option to combine_src.py
  • Emit a file/line metadata file into dist/src/metadata.json
  • Disable line directives in dist combine_src.py step
  • Add an utility to resolve a combined source line into original file/line using dist/src/metadata.json
  • Drop #line directives, or provide both combined source files? => include both, as both variants have relevant use cases
  • Dist documentation changes
  • Other documentation changes; website note and point to wiki?

@svaarala svaarala added this to the v1.4.0 milestone Sep 17, 2015

@svaarala

This comment has been minimized.

Owner

svaarala commented Sep 17, 2015

The file/line metadata map is useful for Duktape development: with a combined source traceback line it can resolved to an original source file/line, e.g.:

$ python util/resolve_combined_lineno.py dist/src/metadata.json 12345
duktape.c:12345 -> duk_builtins.c:793
@svaarala

This comment has been minimized.

Owner

svaarala commented Sep 17, 2015

I'll give a few days before merging this so that if anyone has a strong opinion one way or another they'll have some time to comment.

The concrete impacts of this change are:

  • Debuggers will be less confused about file/line and source file mappings. The debugger will step through the combined source and not the individual sources as e.g. gdb did before.
  • Some packaging tools (see #362) should work better without changes.
  • Error tracebacks will contain duktape.c and a combined line number rather than uncombined source file/line.

To illustrate the traceback issue, before the change:

$ ./duk
((o) Duktape 1.3.99 (v1.3.0-45-g22a33b2-dirty)
duk> decodeURIComponent('%ff')
URIError: invalid input
    duk_bi_global.c:343
    decodeURIComponent  native strict preventsyield
    global input:1 preventsyield

After the change:

$ ./duk
((o) Duktape 1.3.99 (v1.3.0-42-ge19dec5-dirty)
duk> decodeURIComponent('%ff')
URIError: invalid input
    dist/src/duktape.c:29866
    decodeURIComponent  native strict preventsyield
    global input:1 preventsyield

$ python util/resolve_combined_lineno.py dist/src/metadata.json 29866
duktape.c:29866 -> duk_bi_global.c:343

The impact for troubleshooting is that if the Duktape source file/line matters, you'll need to figure out the uncombined file/line from the combined line somehow. The pull adds a utility to do that, and adds the necessary metadata into the dist, so that this will be relatively easy at least for official releases.

@fatcerberus

This comment has been minimized.

Contributor

fatcerberus commented Sep 17, 2015

By the way MSVC behaves similarly to gdb, which threw me off at first: it won't step into duktape.c but instead prompts for the location of the original source files. Normally I don't mind but it can become an issue when prototyping changes directly in the combined source as then the two copies don't match.

On the other hand, it adds an extra level of indirection when deciphering debug prints. So there are pros and cons, and I'm not really sure which way is better.

@svaarala

This comment has been minimized.

Owner

svaarala commented Sep 17, 2015

For application programming the impact in tracebacks should be mostly minimal: tracebacks have a function .name too (decodeURIComponent() above) which usually identifies the relevant internal function quite well for troubleshooting.

With lightfuncs it's a bit different because the virtual lightfunc .name just contains a raw pointer and flags but no function name. This mostly matters when using DUK_OPT_LIGHTFUNC_BUILTINS because then almost all built-ins will be lightfuncs.

@fatcerberus

This comment has been minimized.

Contributor

fatcerberus commented Sep 18, 2015

Hm, debugging may actually be hindered by this: It seems MSVC doesn't like source files with more than 64k lines:
https://connect.microsoft.com/VisualStudio/feedback/details/728010/visual-studio-10-c-debugger-doesnt-work-on-files-with-64k-lines

@svaarala

This comment has been minimized.

Owner

svaarala commented Sep 18, 2015

That sounds vaguely familiar. So, without adding both combined source variants we'll have to choose between either:

  • Causing issues with some packaging tools
  • Causing issues with MSVC debugger

We can add both file variants into the distributable if that's the only workable option though. It's just a bit awkward to explain. Concretely this would mean either adding a third source directory (so e.g. src/, src-no-line-directive/, and src-separate/, maybe abbreviated) or add a second file into src/ (e.g. src/duktape.c-no-line-directive or src/duktape-no-line-directive.c).

@glima glima referenced this pull request Sep 22, 2015

Closed

Lines #3

@glima

This comment has been minimized.

glima commented Oct 16, 2015

No consensus here yet? :)

@svaarala

This comment has been minimized.

Owner

svaarala commented Oct 16, 2015

Any choice will lead to grumbling so I've yet to determine the path of least grumbling ;)

I'll try to get this merged soonish, I've been busy with some other stuff.

@svaarala

This comment has been minimized.

Owner

svaarala commented Nov 3, 2015

Ok, so as a compromise the pull now provides the following dist source structure:

  • src/duktape.c: current single source version with #line directives
  • src-noline/duktape.c: variant without #line directives, directory also contains headers etc
  • src-separate/: current separate source version

The dist README has this note:

* ``src-noline/``: contains a variant of ``src/duktape.c`` with no ``#line``
  directives which is preferable for some users.  See discussion in
  https://github.com/svaarala/duktape/pull/363.

Hopefully this is a reasonable compromise :)

@svaarala svaarala force-pushed the remove-combined-src-line-directives branch Nov 3, 2015

@fatcerberus

This comment has been minimized.

Contributor

fatcerberus commented Nov 3, 2015

This seems reasonable. I like that you chose to put it in a separate directory, keeps things neat.

@svaarala

This comment has been minimized.

Owner

svaarala commented Nov 3, 2015

Yeah, I thought it best not to change the src/ directory; someone may be building with the wildcard src/*.c so having two .c files would have been an unwelcome surprise.

@svaarala svaarala force-pushed the remove-combined-src-line-directives branch to ffcf499 Nov 4, 2015

@svaarala svaarala changed the title from Remove combined source #line directives to Add a combined source without #line directives into the dist package Nov 4, 2015

svaarala added a commit that referenced this pull request Nov 4, 2015

Merge pull request #363 from svaarala/remove-combined-src-line-direct…
…ives

Add a combined source without #line directives into the dist package

@svaarala svaarala merged commit 1529bec into master Nov 4, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@svaarala svaarala deleted the remove-combined-src-line-directives branch Nov 4, 2015

@svaarala svaarala removed the mergeready label Nov 4, 2015

@glima

This comment has been minimized.

glima commented Nov 4, 2015

Thanks, everyone. I'll be updating for the next release on Soletta soon.

@glima

This comment has been minimized.

glima commented Nov 12, 2015

I'm a bit confused -- are there any releases contemplating this change yet?

@fatcerberus

This comment has been minimized.

Contributor

fatcerberus commented Nov 12, 2015

It's in master, which means it will be in the next Duktape release (1.4.0).

@glima

This comment has been minimized.

glima commented Nov 12, 2015

Thanks a lot :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment