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

building with recursive language dependencies should show a pretty error #2101

Closed
brandondiamond opened this issue Aug 13, 2019 · 5 comments · Fixed by #2312
Closed

building with recursive language dependencies should show a pretty error #2101

brandondiamond opened this issue Aug 13, 2019 · 5 comments · Fixed by #2312
Labels
package/build Issues relating to npm or packaging

Comments

@brandondiamond
Copy link

I've been looking into improving the dart language definition; to get started, I built highlight.js locally for dart and received the following error (see below).

After some investigation, it seems that this is caused by the dependency on markdown which, in turn, depends on xml. This appears to be the only dependency chain in the language definition files. I haven't investigated in depth, but it seems that the build tools are currently unable to handle recursive dependencies.

  1 [I] username@username highlight.js> node tools/build.js dart
  2 Starting build.
  3 Copying documentation.
  4 Writing documentation.
  5 Generating demo.
  6 Building highlight.js pack file.
  7 /home/username/githubext/highlight.js/tools/tasks.js:35
  8     if(!object.processed) {
  9                ^
 10 
 11 TypeError: Cannot read property 'processed' of undefined
 12     at pushInBlob (/home/username/githubext/highlight.js/tools/tasks.js:35:16)
 13     at /home/username/githubext/highlight.js/tools/tasks.js:47:9
 14     at arrayEach (/home/username/githubext/highlight.js/node_modules/lodash/lodash.js:516:11)
 15     at Function.forEach (/home/username/githubext/highlight.js/node_modules/lodash/lodash.js:9342:14)
 16     at /home/username/githubext/highlight.js/tools/tasks.js:45:9
 17     at /home/username/githubext/highlight.js/node_modules/lodash/lodash.js:4905:15
 18     at baseForOwn (/home/username/githubext/highlight.js/node_modules/lodash/lodash.js:2990:24)
 19     at /home/username/githubext/highlight.js/node_modules/lodash/lodash.js:4874:18
 20     at Function.forEach (/home/username/githubext/highlight.js/node_modules/lodash/lodash.js:9342:14)
 21     at Queue.tasks.reorderDeps (/home/username/githubext/highlight.js/tools/tasks.js:41:5)
 22     at Queue._dispatch (/home/username/githubext/highlight.js/node_modules/gear/lib/queue.js:101:18)
 23     at Array.<anonymous> (/home/username/githubext/highlight.js/node_modules/gear/lib/tasks/tasks.js:34:18)
 24     at listener (/home/username/githubext/highlight.js/node_modules/async/lib/async.js:493:46)
 25     at /home/username/githubext/highlight.js/node_modules/async/lib/async.js:444:17
 26     at Array.forEach (<anonymous>)
 27     at _each (/home/username/githubext/highlight.js/node_modules/async/lib/async.js:46:24)
 28 

@egor-rogov
Copy link
Collaborator

I haven't investigated either, but seems that node tools/build.js dart markdown works fine.

@brandondiamond
Copy link
Author

Ah, good point. I was going to speculate that this is actually working as intended, but it seems that other languages with dependencies don't require their dependencies to be specified to the tool -- so this does look to be a breakage.

@joshgoebel
Copy link
Member

joshgoebel commented Oct 4, 2019

but it seems that other languages with dependencies don't require their dependencies to be specified to the tool -- so this does look to be a breakage.

Can you provide examples of this "non-requirement"? If you don't build the dependencies then the languages are going to break IME. This is true of at least SQF/Cpp for sure because I've worked with it myself (although there is a PR to remove the dependency).

Or I think you're talking specifically about the BUILD rather than usage of the library...

@joshgoebel
Copy link
Member

This should probably be considered when/if we get around to reworking the build system?

@joshgoebel joshgoebel changed the title Recursive "Requires:" not working Recursive language dependencies (Requires:) blows up during build process Oct 7, 2019
@joshgoebel joshgoebel added package/build Issues relating to npm or packaging and removed other labels Oct 11, 2019
@joshgoebel joshgoebel added this to the New build pipeline milestone Oct 16, 2019
@joshgoebel joshgoebel modified the milestones: Build pipeline, 9.15.12 Oct 24, 2019
@joshgoebel joshgoebel changed the title Recursive language dependencies (Requires:) blows up during build process building with recursive language dependencies should show a pretty error Oct 24, 2019
@joshgoebel joshgoebel removed the bug label Oct 24, 2019
@joshgoebel joshgoebel added this to .12 Plan in Highlight.js Oct 25, 2019
@joshgoebel joshgoebel moved this from .12 Plan to .12 Build in Highlight.js Oct 25, 2019
@joshgoebel joshgoebel modified the milestones: 9.17, 9.18 Dec 6, 2019
@joshgoebel joshgoebel moved this from Build System / Packaging to Has open PR in Highlight.js Dec 7, 2019
@joshgoebel joshgoebel modified the milestones: 9.18, 10.0 Dec 23, 2019
@joshgoebel
Copy link
Member

I just adding a commit to my v10 prep-work PR that allows the existing build system to go 2 levels deep with it's dependency chaining (had to do it to get C/C++ split out) - and this should be solved completely by the new build system in #2312 . Closing issue.

Highlight.js automation moved this from Has open PR to Done Jan 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package/build Issues relating to npm or packaging
Projects
No open projects
Highlight.js
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants