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

Node Build Script #550

Merged
merged 22 commits into from
Oct 9, 2014
Merged

Node Build Script #550

merged 22 commits into from
Oct 9, 2014

Conversation

sourrust
Copy link
Member

Implemented the old python script in node to rely on one dependency -- that being node.js. I made the CLI identical to the old one so it won't be that difficult to change from the old to the new.

I'm pretty sure I've got the builds correctly but I'm making this pull request to stamp on any issues.

This mimics the old build script in python so there is minimal
transitioning from old to new.

As an example, the node build:

  python3 tools/build.py -t node

  node tools/build.js -t node
This is used for browser, amd, and part of the cdn build.
@isagalaev
Copy link
Member

Yeah, I was thinking about the same since @chriseidhof's danced around Python version for Travis. There's one reason why we can't just switch immediately to this: the Python script as used as a library within highlightjs.org to generate custom downloads. I should look into it to see if it's feasible to call an external process instead.

@chriseidhof
Copy link
Contributor

Oh awesome! The python and node dependency was a bit weird. Great that it'll be all javascript now.

@sourrust sourrust mentioned this pull request Aug 28, 2014
@sourrust
Copy link
Member Author

sourrust commented Oct 3, 2014

@isagalaev any progress on getting this working for highlightjs.org?

@isagalaev
Copy link
Member

Not yet, I was invested into another project for the past few weeks. I should get back to highlight.js next week. Though I was planning to finish first the new demo "page" in #542.

@isagalaev
Copy link
Member

As it happens, while writing a vast explanation of how non-trivial that would be for highlightjs.org I realized that it's actually quite doable :-). Here's the edited out version:

There are a few things for which highlightjs.org uses the build script as a library:

  • Building pre-compressed language files to avoid compressing the custom build on each download.
  • Glueing pre-compressed files together, making sure that the build file is exactly the same as the one build by build.py -tbrowser, this is the only reason for the compressed argument in wrap_language and glue_files.
  • Listing languages by category with their proper human-readable names (as displayed on the download page).

As I realize now, the first two items were there before we had the CDN build target and it apparently does exactly what I need, I will just have to concatenate all the files together which is easy and (hopefully) future-proof.

The listing languges part can (and probably should) be fixed by moving category definition from the build script into language headers (Category: common, ..., ...). The site and the build script will probably have some duplicated logic parsing those headers but as long as we don't change the format incompatibly (which we shouldn't) I can live with this, given that we gain so much simplification by streamlining the tool chain.

So here's the proposed plan:

  • @sourrust does the category change: adding Category: common to 22 language files and using that in the script instead of CATEGORIES.
  • @isagalaev updates the site to use the CDN build and call the build script in a few other places instead of calling Python functions. Since CLI remains the same, I can do this using the current 8.2 release code.

Sounds good?

@isagalaev
Copy link
Member

BTW, it probably makes sense to do all this after the CDN auto-update thing and the release of 8.3.

@isagalaev
Copy link
Member

Progress update: I've updated the site, it doesn't depend anymore on the Python code, needs only to switch to the new shell command invocation.

BTW, it's impressive how fast the builds are now, when there's no JVM involved :-)

@chriseidhof
Copy link
Contributor

Really cool stuff! Thanks for mentioning me in the blogpost, that wasn't
really necessary (I think I added about 5 lines of code, haha).

On Tue, Oct 7, 2014 at 9:18 PM, Ivan Sagalaev [email protected]
wrote:

Progress update: I've updated the site, it doesn't depend anymore on the
Python code, needs only to switch to the new shell command invocation.

BTW, it's impressive how fast the builds are now, when there's no JVM
involved :-)


Reply to this email directly or view it on GitHub
#550 (comment)
.

Chris Eidhof

@isagalaev
Copy link
Member

Thanks for mentioning me in the blogpost, that wasn't really necessary (I think I added about 5 lines of code, haha).

You removed the headache of doing research and testing the result. The fact of taking care of the overall job is usually worth much, much more than the actual amount of work required. So thank you!

@sourrust
Copy link
Member Author

sourrust commented Oct 8, 2014

The listing languges part can (and probably should) be fixed by moving category definition from the build script into language headers (Category: common, ..., ...).

Since both python and javascript can parse JSON easily, wouldn't it be better to just put these categories into a separate JSON file?

@isagalaev
Copy link
Member

I was hoping to finally unify meta data in one place, and the file's header seems a good place. Aren't you parsing them anyway for "Requires"?

@isagalaev isagalaev added the enhancement An enhancement or new feature label Oct 8, 2014
@isagalaev
Copy link
Member

Tentatively nominating it for 8.3 as I feel it shouldn't take long to iron out now. @sourrust feel free to drop the milestone, I'll release the 8.3 right away then.

@isagalaev isagalaev added this to the 8.3 milestone Oct 8, 2014
@sourrust
Copy link
Member Author

sourrust commented Oct 9, 2014

Aren't you parsing them anyway for "Requires"?

I am, but I thought it would be a simpler solution. Either way, I got it working with Category in the language headers.

@isagalaev isagalaev merged commit b588ff7 into master Oct 9, 2014
@sourrust sourrust mentioned this pull request Dec 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants