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

Infrastructure #3

Merged
merged 2 commits into from
Sep 15, 2017
Merged

Infrastructure #3

merged 2 commits into from
Sep 15, 2017

Conversation

fitzgen
Copy link
Collaborator

@fitzgen fitzgen commented Aug 10, 2017

Common infrastructure types we will need: an Error type, an Options builder`, CLI arg parsing.

r? @tschneidereit

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 10, 2017

Ah ok, CI is failing because we need to have newer GCC. Will update in a bit.

Copy link
Collaborator

@tschneidereit tschneidereit left a comment

Choose a reason for hiding this comment

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

Looks good, apart from the nit below. r=me with that and the GCC issue fixed.

src/lib.rs Outdated
extern crate futures;
extern crate tokio_core;

pub mod cli;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's start this off with proper separation and not include this in lib.rs: the library shouldn't know about arg parsing. (Yes, I realize this is all scaffolding, but this is strictly code we'd remove from this file later on anyway.)

Copy link
Collaborator Author

@fitzgen fitzgen Aug 10, 2017

Choose a reason for hiding this comment

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

You want to move the cli mod into src/bin/starling.rs, or you don't want the module to be pub, or you want it to be in a whole new crate? It's not clear to me what you're requesting here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really didn't make that clear at all, sorry. I think the use of cli shouldn't be in src/lib.rs. src/bin/starling.rs sounds like a good place for it.

@fitzgen fitzgen force-pushed the infrastructure branch 6 times, most recently from f11a332 to 5feda29 Compare August 14, 2017 19:35
@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 14, 2017

g++-5: internal compiler error: Killed (program cc1plus)

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 15, 2017

g++-6: internal compiler error: Killed (program cc1plus)

Crap.

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 15, 2017

Wait, I think the ICEs are just from the process getting killed in the middle of compilation.

I think this is the real culprit:

warning: build failed, waiting for other jobs to finish...
warning: variable does not need to be mutable
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/syntex_syntax-0.58.1/src/ext/tt/macro_parser.rs:204:63
    |
204 |     fn n_rec<I: Iterator<Item=Rc<NamedMatch>>>(m: &TokenTree, mut res: &mut I,
    |                                                               ^^^^^^^
    |
note: lint level defined here
   --> /home/travis/.cargo/registry/src/github.com-1ecc6299db9ec823/syntex_syntax-0.58.1/src/lib.rs:23:9
    |
23  | #![deny(warnings)]
    |         ^^^^^^^^

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 15, 2017

And that is actually triggered by a cargo bug: serde-deprecated/syntex#119

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 15, 2017

Er, rust-lang/cargo#3823

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 15, 2017

Hrm, it seems we're still ICEing:

{standard input}: Assembler messages:
{standard input}:16702: Warning: end of file not at end of a line; newline inserted
{standard input}:17014: Error: unknown pseudo-op: `.p2'
{standard input}: Error: open CFI at the end of file; missing .cfi_endproc directive
g++-6: internal compiler error: Killed (program cc1plus)

@tschneidereit
Copy link
Collaborator

@fitzgen, is this something we should talk to cargo folks about, or just something that needs a little more digging?

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 21, 2017

@fitzgen, is this something we should talk to cargo folks about, or just something that needs a little more digging?

We were running into an issue where -vv to enable "very verbose" mode was also making warnings into errors in our dependencies, which was breaking the build. That is fixed but not released, but we can work around it by building with only -v instead of -vv.

The thing keeping this from landing right now, is that AFAICT those gcc ICEs are real, and I haven't been able to find a gcc version that doesn't ICE yet. This is a bit confusing to me, since earlier/other PRs with this same mozjs have all been A-OK with gcc.

Digging in more.

@fitzgen fitzgen force-pushed the infrastructure branch 2 times, most recently from a45abd7 to 4138309 Compare August 22, 2017 16:43
@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 22, 2017

So I think the ICEs are an unrelated goose chase.

Even without -v, syntex_syntax is failing to compile:

error: Could not compile `syntex_syntax`.

And that's all the information that's getting spit out in the logs...

And yet it works just fine locally.

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 22, 2017

On IRC acrichto says that this might be cargo swallowing the error, perhaps because of OOM.

@tschneidereit
Copy link
Collaborator

On IRC acrichto says that this might be cargo swallowing the error, perhaps because of OOM.

Ugh. Can you try compiling with fewer threads to see if that's the case?

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 22, 2017

Trying with CARGO_BUILD_JOBS=4.

@fitzgen fitzgen force-pushed the infrastructure branch 3 times, most recently from 8232a1a to 9ea0c2c Compare August 22, 2017 22:02
@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 22, 2017

Ok, by restricting to one parallel jobs everywhere, 1/4 (so far) CI tasks finished successfully without OOMing, while 3/4 hit the travis CI time limit.

Time to slowly introduce more parallel jobs again.

@fitzgen fitzgen force-pushed the infrastructure branch 2 times, most recently from bb45d3c to 215eb44 Compare August 23, 2017 00:27
@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 23, 2017

4 parallel jobs = some are timing out; 8 parallel jobs = OOM

@fitzgen
Copy link
Collaborator Author

fitzgen commented Aug 23, 2017

6 parallel jobs is also OOMing.

@fitzgen fitzgen mentioned this pull request Sep 14, 2017
Merged
@fitzgen
Copy link
Collaborator Author

fitzgen commented Sep 15, 2017

Going to just merge this, since we are planning on dealing with CI later, and other PRs build on top of this.

@fitzgen fitzgen merged commit ea89e1e into starlingjs:master Sep 15, 2017
@fitzgen fitzgen deleted the infrastructure branch September 15, 2017 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants