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

pull extra::{serialize, ebml} into a separate libserialize crate #11984

Merged
merged 1 commit into from Feb 5, 2014

Conversation

@olsonjeffery
Copy link
Contributor

commented Feb 2, 2014

  • extra::json didn't make the cut, because of extra::json's required
    dep on extra::TreeMap. If/when extra::TreeMap moves out of extra,
    then extra::json could move into libserialize
  • libextra, libsyntax and librustc depend on the newly created
    libserialize
  • The extensions to various extra types like DList, RingBuf, TreeMap
    and TreeSet for Encodable/Decodable were moved into the respective
    modules in extra
  • There is some trickery, evident in src/libextra/lib.rs where a stub
    of extra::serialize is set up (in src/libextra/serialize.rs) for
    use in the stage0 build, where the snapshot rustc is still making
    deriving for Encodable and Decodable point at extra. Big props to
    @huonw for help working out the re-export solution for this
  • @pcwalton's change in 449a7a8 didn't sneak back in
pub use self::serialize::{Decoder, Encoder, Decodable, Encodable,
DecoderHelpers, EncoderHelpers};

pub mod serialize;

This comment has been minimized.

Copy link
@huonw

huonw Feb 2, 2014

Member

Because of the reexport, I think this should be private (assuming that you're reexporting everything).

#[forbid(non_camel_case_types)];


use std::at_vec;

This comment has been minimized.

Copy link
@huonw

huonw Feb 2, 2014

Member

I think you may've undo the changes from huonw@2ed980f#diff-1 (or, at least, not rebased on top of them).

@sfackler

This comment has been minimized.

Copy link
Member

commented Feb 2, 2014

Should hex and base64 move here as well?

@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2014

@sfackler not sure.. i was concentrating on all of the stuff that falls out of Decodable/Encodable, but I guess you could fairly include those in a more expansive defintion of serialization. It also occurred to me that this could be a home for stuff related to string encoding in rust. But maybe that's a bridge too far.

#[license = "MIT/ASL2"];
#[allow(missing_doc)];
#[forbid(non_camel_case_types)];
#[feature(globs, managed_boxes)];

This comment has been minimized.

Copy link
@huonw

huonw Feb 2, 2014

Member

I think the standard thing for the extra dissolution is to attempt to remove feature(globs); is that possible here?

This comment has been minimized.

Copy link
@olsonjeffery

olsonjeffery Feb 2, 2014

Author Contributor

I'll take a whack at it.. in the morning!

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 2, 2014

Nice work, thanks for doing this!

I also feel as if hex/base64 should be candidates for this crate, and perhaps the crate should be called libencode or libencoding? (just a thought)

@omasanori

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2014

Why don't you separate serialize and ebml? I think we don't need to link to EBML or JSON library when we are developing another serialization library.

@flaper87

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2014

@omasanori yours is a good point. However, it also depends on how granular we want to make this extra explosion. It makes sense to group different serialization protocols under the same crate / namespace.

@alexcrichton libencode +1

@omasanori

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2014

@flaper87 well, probably we don't want to see hundreds of libraries in mozilla/rust.

I hope the @rust-lang GitHub group will host non-fundamental official libraries (just like what @clojure does) but at this time we should think about the number of libraries in mozilla/rust... hmm...

@huonw

This comment has been minimized.

Copy link
Member

commented Feb 2, 2014

I don't particularly see a reason to separate ebml etc. from libserialize: in a statically linked release build (when the bloat is relevant and at risk of being the worst) the unused code will be removed from the binary.

(Also, this needs a rebase.)

@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2014

As stated above, I think extra::json should move out as soon as extra::treemap::Treemap leaves extra, as well.

As for the library rename: meh, whatever. I think libencode is loaded because it also implies all of the whatwg encoding ball of spaghetti (and there's already a rust-encoding community library) that we approached but never pulled the trigger on in the past (cc @SimonSapin @lifthrasiir ). So are we prepared to bring that stuff in now, or in the future?

I can bring in hex and base64 if that's the concensus. With that in mind, I would like there to be some boundaries around how long this PR goes on. These big library changes are a PITA to keep rebased.

@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2014

I still need to remove globs, hopefully will knock that out before the SB.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 2, 2014

If you don't have the time, then it's perfectly fine to not move hex/base64 right now. I would think that the only thing blocking this from landing is feature(globs) (unless that's too painful to remove).

@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 2, 2014

I have no issues adding them in a later PR. plus would like to start a converastion about data structures like TreeMap, so that I could move json out, too.

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 2, 2014

I would expect libcontainer to come into existence which contains all the fun containers that we've come to have in libextra. "container" is a bit of a long name though...

bors added a commit that referenced this pull request Feb 2, 2014
- `extra::json` didn't make the cut, because of `extra::json`'s required
   dep on `extra::TreeMap`. If/when `extra::TreeMap` moves out of `extra`,
   then `extra::json` could move into `libserialize`
- `libextra`, `libsyntax` and `librustc` depend on the newly created
  `libserialize`
- The extensions to various `extra` types like `DList`, `RingBuf`, `TreeMap`
  and `TreeSet` for `Encodable`/`Decodable` were moved into the respective
  modules in `extra`
- There is some trickery, evident in `src/libextra/lib.rs` where a stub
  of `extra::serialize` is set up (in `src/libextra/serialize.rs`) for
  use in the stage0 build, where the snapshot rustc is still making
  deriving for `Encodable` and `Decodable` point at extra. Big props to
  @huonw for help working out the re-export solution for this
- @pcwalton's change in 449a7a8 didn't sneak back in
@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2014

rebased to deal w/ libterm merge conflict + fix for stage1 libsyntax

@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2014

this should be ready to go.. r? @alexcrichton

bors added a commit that referenced this pull request Feb 5, 2014
- `extra::json` didn't make the cut, because of `extra::json`'s required
   dep on `extra::TreeMap`. If/when `extra::TreeMap` moves out of `extra`,
   then `extra::json` could move into `libserialize`
- `libextra`, `libsyntax` and `librustc` depend on the newly created
  `libserialize`
- The extensions to various `extra` types like `DList`, `RingBuf`, `TreeMap`
  and `TreeSet` for `Encodable`/`Decodable` were moved into the respective
  modules in `extra`
- There is some trickery, evident in `src/libextra/lib.rs` where a stub
  of `extra::serialize` is set up (in `src/libextra/serialize.rs`) for
  use in the stage0 build, where the snapshot rustc is still making
  deriving for `Encodable` and `Decodable` point at extra. Big props to
  @huonw for help working out the re-export solution for this
- @pcwalton's change in 449a7a8 didn't sneak back in
@olsonjeffery

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2014

not sure what this is about.. failure on freebsd.

compile_and_link: x86_64-unknown-freebsd/stage2/lib/rustlib/x86_64-unknown-freebsd/lib/librun_pass_stage2.so
warning: ignoring specified output filename for library.
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:16:5: 16:10 error: unresolved import. maybe a missing `extern mod extra`?
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:16 use extra::json;
                                                                                                     ^~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:16:5: 16:16 error: failed to resolve import `extra::json`
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:16 use extra::json;
                                                                                                     ^~~~~~~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:17:5: 17:14 error: unresolved import. maybe a missing `extern mod serialize`?
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:17 use serialize::Decodable;
                                                                                                     ^~~~~~~~~
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:17:5: 17:25 error: failed to resolve import `serialize::Decodable`
/home/rustbuild/src/rust-buildbot/slave/auto-bsd-64-opt/build/src/test/run-pass/issue-4036.rs:17 use serialize::Decodable;
                                                                                                     ^~~~~~~~~~~~~~~~~~~~
error: aborting due to 4 previous errors
gmake: *** [x86_64-unknown-freebsd/stage2/lib/rustlib/x86_64-unknown-freebsd/lib/librun_pass_stage2.so] Error 101
program finished with exit code 2

Those lines are immediately preceeded by the extern mod lines... something in the env/build config?

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Feb 5, 2014

This is likely the check-fast suite, feel free to xfail-fast those tests.

- `extra::json` didn't make the cut, because of `extra::json` required
   dep on `extra::TreeMap`. If/when `extra::TreeMap` moves out of `extra`,
   then `extra::json` could move into `serialize`
- `libextra`, `libsyntax` and `librustc` depend on the newly created
  `libserialize`
- The extensions to various `extra` types like `DList`, `RingBuf`, `TreeMap`
  and `TreeSet` for `Encodable`/`Decodable` were moved into the respective
  modules in `extra`
- There is some trickery, evident in `src/libextra/lib.rs` where a stub
  of `extra::serialize` is set up (in `src/libextra/serialize.rs`) for
  use in the stage0 build, where the snapshot rustc is still making
  deriving for `Encodable` and `Decodable` point at extra. Big props to
  @huonw for help working out the re-export solution for this

extra: inline extra::serialize stub

fix stuff clobbered in rebase + don't reexport serialize::serialize

no more globs in libserialize

syntax: fix import of libserialize traits

librustc: fix bad imports in encoder/decoder

add serialize dep to librustdoc

fix failing run-pass tests w/ serialize dep

adjust uuid dep

more rebase de-clobbering for libserialize

fixing tests, pushing libextra dep into cfg(test)

fix doc code in extra::json

adjust index.md links to serialize and uuid library
@alexcrichton

This comment has been minimized.

Copy link

commented on b8852e8 Feb 5, 2014

r+

@bors

This comment has been minimized.

Copy link
Contributor

commented on b8852e8 Feb 5, 2014

saw approval from alexcrichton
at olsonjeffery@b8852e8

This comment has been minimized.

Copy link
Contributor

replied Feb 5, 2014

merging olsonjeffery/rust/libserialize = b8852e8 into auto

This comment has been minimized.

Copy link
Contributor

replied Feb 5, 2014

olsonjeffery/rust/libserialize = b8852e8 merged ok, testing candidate = 55684ba

This comment has been minimized.

Copy link
Contributor

replied Feb 5, 2014

This comment has been minimized.

Copy link
Contributor

replied Feb 5, 2014

fast-forwarding master to auto = 55684ba

bors added a commit that referenced this pull request Feb 5, 2014
- `extra::json` didn't make the cut, because of `extra::json`'s required
   dep on `extra::TreeMap`. If/when `extra::TreeMap` moves out of `extra`,
   then `extra::json` could move into `libserialize`
- `libextra`, `libsyntax` and `librustc` depend on the newly created
  `libserialize`
- The extensions to various `extra` types like `DList`, `RingBuf`, `TreeMap`
  and `TreeSet` for `Encodable`/`Decodable` were moved into the respective
  modules in `extra`
- There is some trickery, evident in `src/libextra/lib.rs` where a stub
  of `extra::serialize` is set up (in `src/libextra/serialize.rs`) for
  use in the stage0 build, where the snapshot rustc is still making
  deriving for `Encodable` and `Decodable` point at extra. Big props to
  @huonw for help working out the re-export solution for this
- @pcwalton's change in 449a7a8 didn't sneak back in
@bors bors closed this Feb 5, 2014
@bors bors merged commit b8852e8 into rust-lang:master Feb 5, 2014
1 check passed
1 check passed
default all tests passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.