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.

Sign up for GitHub

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

Jump to bottom

Add minification process #50632

Merged
merged 2 commits into from
May 15, 2018
Merged

Add minification process #50632

merged 2 commits into from
May 15, 2018

Conversation

GuillaumeGomez
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2018
@GuillaumeGomez
Copy link
Member Author

For the record, here's the error:

Building rustdoc for stage1 (x86_64-apple-darwin)
   Compiling pulldown-cmark v0.1.2
   Compiling tempdir v0.3.7
   Compiling minifier v0.0.4
   Compiling rustdoc v0.0.0 (file:///Users/imperio/rust/rust/src/librustdoc)
   Compiling rustdoc-tool v0.0.0 (file:///Users/imperio/rust/rust/src/tools/rustdoc)
    Finished release [optimized] target(s) in 54.79 secs
dyld: Library not loaded: @rpath/libminifier-ff582a1ef3309921.dylib
  Referenced from: /Users/imperio/rust/rust/build/x86_64-apple-darwin/stage1/bin/rustdoc
  Reason: image not found

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log. 
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
/usr/local/lib/python2.7/dist-packages/pip/_vendor/requests/packages/urllib3/util/ssl_.py:122: InsecurePlatformWarning: A true SSLContext object is not available. This prevents urllib3 from configuring SSL appropriately and may cause certain SSL connections to fail. You can upgrade to a newer version of Python to solve this. For more information, see https://urllib3.readthedocs.io/en/latest/security.html#insecureplatformwarning.
  InsecurePlatformWarning
  Downloading https://files.pythonhosted.org/packages/18/61/4e0f977cfe063188d73622a91cab8b8b409b662f422303fc687f362d941f/awscli-1.15.18-py2.py3-none-any.whl (1.3MB)
    0% |▎                               | 10kB 16.2MB/s eta 0:00:01
    1% |▌                               | 20kB 1.9MB/s eta 0:00:01
    2% |▉                               | 30kB 2.2MB/s eta 0:00:01
    3% |█                               | 40kB 2.0MB/s eta 0:00:01

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@ollie27
Copy link
Member

ollie27 commented May 10, 2018

Is there a reason you are tying to build it as a dylib? https://github.com/GuillaumeGomez/minifier-rs/blob/81851f08301dd149c2de25889ade81291ca6de74/Cargo.toml#L23

@GuillaumeGomez
Copy link
Member Author

Old (and bad apparently) habit. Good catch, thanks a lot!

@GuillaumeGomez
Copy link
Member Author

Fixed the initial issue, now just need to make the minification process work as expected. :)

@GuillaumeGomez GuillaumeGomez force-pushed the minification branch 2 times, most recently from d0011a1 to 3a51b0a Compare May 11, 2018 22:46
@GuillaumeGomez GuillaumeGomez changed the title [WIP] Add minification process Add minification process May 11, 2018
kennytm
kennytm reviewed May 12, 2018
View reviewed changes
src/librustdoc/html/render.rs Outdated
include_bytes!("static/main.js"),
enable_minification)?;
write_minify(cx.dst.join(&format!("settings{}.js", cx.shared.resource_suffix)),
include_bytes!("static/settings.js"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we minimize these two *.js files at build.rs of rustdoc?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather prefer that we don't. We could but then disabling minification would be a bit more complicated and I'm not sure it's worth it.

ollie27
ollie27 reviewed May 12, 2018
View reviewed changes
src/librustdoc/html/render.rs Outdated
@@ -1020,6 +1028,18 @@ fn write(dst: PathBuf, contents: &[u8]) -> Result<(), Error> {
Ok(try_err!(fs::write(&dst, contents), &dst))
}

fn write_minify(dst: PathBuf, contents: &[u8], enable_minification: bool) -> Result<(), Error> {
if enable_minification {
if let Ok(s) = String::from_utf8(contents.to_vec()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use include_str! instead of include_bytes! for the static files to avoid this UTF-8 check.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@GuillaumeGomez
Copy link
Member Author

Updated following @ollie27's comment.

@QuietMisdreavus
Copy link
Member

Seems good to me. What do you think, @ollie27?

@ollie27
Copy link
Member

ollie27 commented May 15, 2018

Yeah this looks good.

For the std docs it might make more sense for the minification to be done by rustbuild so it can apply to the docs generated by mdBook as well but this seems like a reasonable start anyway.

@GuillaumeGomez
Copy link
Member Author

For now it's experimental. If we have no issue for a given time, we might want to expand this to other tools as well. Thanks for your review!

@bors: r=ollie27

@bors
Copy link
Contributor

bors commented May 15, 2018

📌 Commit e2db0a5 has been approved by ollie27

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2018
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request May 15, 2018
@GuillaumeGomez
Rollup merge of rust-lang#50632 - GuillaumeGomez:minification, r=ollie27
934e37a 
Add minification process

r? @QuietMisdreavus
@GuillaumeGomez GuillaumeGomez mentioned this pull request May 15, 2018
Merged
bors added a commit that referenced this pull request May 15, 2018
@bors
Auto merge of #50769 - GuillaumeGomez:rollup, r=GuillaumeGomez
27acb9b 
Rollup of 11 pull requests

Successful merges:

 - #49767 (Rewrite docs for `std::ptr`)
 - #50399 (save-analysis: handle aliasing imports a bit more nicely)
 - #50594 (Update the man page with additional --print options)
 - #50613 (Migrate the toolstate update bot to rust-highfive)
 - #50632 (Add minification process)
 - #50685 (ci: Add Dockerfile for dist-sparc64-linux)
 - #50691 (rustdoc: Add support for pub(restricted))
 - #50712 (Improve eager type resolution error message)
 - #50720 (Add “Examples” section header in f32/f64 doc comments.)
 - #50733 (Hyperlink DOI against preferred resolver)
 - #50745 (Uncapitalize "You")

Failed merges:
@bors bors merged commit e2db0a5 into rust-lang:master May 15, 2018
@GuillaumeGomez GuillaumeGomez deleted the minification branch May 15, 2018 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ollie27 ollie27 ollie27 left review comments

@kennytm kennytm kennytm left review comments

Assignees

@QuietMisdreavus QuietMisdreavus

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuillaumeGomez @rust-highfive @ollie27 @QuietMisdreavus @bors @kennytm