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

Generate strings instead of AST #239

Merged
merged 3 commits into from Dec 24, 2014

Conversation

@tomaka
Copy link
Collaborator

commented Dec 15, 2014

This change will totally remove the generate_gl_bindings macro, in favor of Cargo build scripts.

If you simply use the gl library, nothing changes.

If, however, you use gl_generator, then instead of:

generate_gl_bindings! {
 api: "gl",
 profile: "core",
 version: "4.5",
 generator: "global",
 extensions: [
 "GL_EXT_texture_filter_anisotropic",
 ],
}

Replace this by:

include!(concat!(env!("OUT_DIR"), "/gl_bindings.rs"));

And create a file named build.rs:

extern crate gl_generator;
extern crate khronos_api;

use std::os;
use std::io::File;

fn main() {
    let dest = Path::new(os::getenv("OUT_DIR").unwrap());

    let mut file = File::create(&dest.join("gl_bindings.rs")).unwrap();

    gl_generator::generate_bindings(gl_generator::GlobalGenerator,
                                                   gl_generator::registry::Ns::Gl,
                                                   khronos_api::GL_XML,
                                                   vec!["GL_EXT_texture_filter_anisotropic".to_string()],
                                                   "4.5", "core",
                                                   &mut file).unwrap();
}

You will also need to tweak your Cargo.toml:

[package]
# ...
build = "build.rs"

[build-dependencies.gl_generator]
git = "https://github.com/bjz/gl-rs"

[dependencies.gl_common]
git = "https://github.com/bjz/gl-rs"

# remove [dependencies.gl_generator]

@tomaka tomaka changed the title Generate strings instead of AST [WIP] Generate strings instead of AST Dec 15, 2014

fn main() {
let dest = Path::new(os::getenv("OUT_DIR").unwrap());

let bindings = gl_generator::generate(gl_generator::generators::global_gen::GlobalGenerator, gl_generator::registry::Ns::Gl, khronos_api::GL_XML, vec![], "4.5", "core");

This comment has been minimized.

Copy link
@cmr

cmr Dec 15, 2014

Collaborator

Can we reexport these as just gl_generators::Global etc?

This comment has been minimized.

Copy link
@brendanzab

brendanzab Dec 16, 2014

Owner

Yes, it would be nice to have them exported at the top level.

@cmr

This comment has been minimized.

Copy link
Collaborator

commented Dec 15, 2014

cc @bvssvni @csherratt @hannobraun this might affect you

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 16, 2014

This is looking awesome! It's a big review though.

Is it under integration tests?

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 16, 2014

I'm wondering if it would be better to be using writers instead of all the strings to cut down on the allocation, but I would rather get this merged soon so that @tomaka doesn't have to deal with breakages when we inevitably have patch changes to the AST.

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 16, 2014

Don't merge this for now. The build command doesn't work on my local machine (missing DLL symbol when executing it), I don't know where it comes from and didn't investigate yet.
I uploaded this to see what travis tells me, but I forgot that nothing was working.

@tomaka tomaka force-pushed the tomaka:ast-to-strings branch 3 times, most recently from 5a5dcd9 to 5c28bbe Dec 16, 2014

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 16, 2014

Investigation wasn't long, it was because I was linking to the rustc and syntax crates.

The only thing not done yet is fixing the tests (which is important, because I noticed that I made a few mistakes in global_gen, but didn't test the other generators yet).

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 16, 2014

Awesome! Thanks again for your hard work @tomaka!

@tomaka tomaka force-pushed the tomaka:ast-to-strings branch 4 times, most recently from 9d384cc to 152ffc0 Dec 17, 2014

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 17, 2014

Because of rust-lang/cargo#1058, the build time of gl-rs is a lot higher than it could be.

Except this, everything is done.

@tomaka tomaka changed the title [WIP] Generate strings instead of AST Generate strings instead of AST Dec 17, 2014

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 17, 2014

Have you tried passing around writers instead of constantly allocating strings?

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 18, 2014

@bjz Done

@tomaka tomaka force-pushed the tomaka:ast-to-strings branch from 8013ee9 to 5140fe3 Dec 18, 2014

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 18, 2014

r?

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 20, 2014

@bvssvni @csherratt @hannobraun: what are your thoughts?

@bvssvni

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2014

+1

@hannobraun

This comment has been minimized.

Copy link

commented Dec 20, 2014

Sorry, no thoughts at the moment. I haven't really had a chance to look into what code generation is supposed to look like post-1.0.

@erickt

This comment has been minimized.

Copy link

commented Dec 21, 2014

I'm working on a proposal for a code generation strategy for 1.0. I'll have it out in a week or so.

@ghost

This comment has been minimized.

Copy link

commented Dec 21, 2014

One surprising effect of this change is that it dramatically increases the build times. gl-rs with this change takes 79 seconds to build, where before it took just 26 seconds on my machine. It's not that big of a deal since you don't recompile gl-rs every time you build, but it does demonstrate the power of the rust plug-in system.

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 21, 2014

@csherratt No, it's only because this line is commented out.
Every time you compile gl-rs, it generates 30 bindings (the real bindings, plus 29 bindings used for the tests), which means that compiling gl-rs will be around 30 times faster when rust-lang/cargo#1058 is fixed.

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 22, 2014

@erickt Any idea what it will look like? How easy will it be to deploy to crates.io?

@erickt

This comment has been minimized.

Copy link

commented Dec 22, 2014

@bjz: the rough idea is to pull the rust 1.0 libsyntax out into a separate crate, and use it to expand syntax extensions and macros into a generated file. Since Rust 1.x+ will be backwards compatible with 1.0, this should be safe to do. You can see my very rough prototype here, specifically the hello_world example. It should be very easy to deploy to crates.io :)

@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 23, 2014

What do you think of @erickt's solution @tomaka? Sorry for the runaround on this - I know you put a lot of time into it. Just making sure that everything is fine before we commit. :)

@tomaka

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 23, 2014

I don't exactly see how that would work in pratice, but as far as I understand:

  • it would still require generating strings (especially if there's no quote! macro)
  • it would still require using a build script

Therefore I don't see any advantage in using @erickt's prototype for gl-rs.

@cmr

This comment has been minimized.

Copy link
Collaborator

commented Dec 23, 2014

@tomaka I basically agree right now, :shipit:

@tomaka tomaka force-pushed the tomaka:ast-to-strings branch from 5140fe3 to 02afee7 Dec 23, 2014

@erickt

This comment has been minimized.

Copy link

commented Dec 23, 2014

No worries, what you're doing is fine. syntex is really will have two goals:

  1. help lazy people like me expose stuff I already wrote without having to write my own code generator.
  2. help people write syntax extensions that are a small part of a larger file. For example, #[deriving(Foo)] is expected to be expanded in the same file with a bunch of other user written code.

Since it looks like you've already rewritten the code generator, and it looks like you're generating the whole file, then there's no need to use something like syntex.

@ghost

This comment has been minimized.

Copy link

commented Dec 23, 2014

:shipit:

1 similar comment
@bvssvni

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2014

:shipit:

brendanzab added a commit that referenced this pull request Dec 24, 2014

Merge pull request #239 from tomaka/ast-to-strings
Generate strings instead of AST

@brendanzab brendanzab merged commit efd4aca into brendanzab:master Dec 24, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@brendanzab

This comment has been minimized.

Copy link
Owner

commented Dec 24, 2014

Thanks for all the feedback everyone!

@tomaka tomaka deleted the tomaka:ast-to-strings branch Dec 24, 2014

@Ryman Ryman referenced this pull request Dec 25, 2014

Merged

Rustup #15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.