Skip to content

Conversation

@gkoz
Copy link
Contributor

@gkoz gkoz commented Oct 10, 2015

Fixes #1980

@rust-highfive
Copy link

r? @wycats

(rust_highfive has picked a reviewer for you, use r? to override)

@gkoz
Copy link
Contributor Author

gkoz commented Oct 10, 2015

r? @alexcrichton

@rust-highfive rust-highfive assigned alexcrichton and unassigned wycats Oct 10, 2015
Copy link
Member

Choose a reason for hiding this comment

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

You'll want to do a hash lookup here to ensure that you're doing the right thing in a cross-compilation scenario. Right now this ignores the Kind which is an important distinction for target/host compiles.

The Kind to use in the key should be present in unit.kind right before the work is created, and that should do the trick!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I wasn't sure whether the kind mattered here.

@alexcrichton
Copy link
Member

Awesome, thanks @gkoz!

@gkoz
Copy link
Contributor Author

gkoz commented Oct 11, 2015

Updated for both comments.

@gkoz
Copy link
Contributor Author

gkoz commented Oct 11, 2015

@alexcrichton looks like you don't have `Rolling builds" enabled in appveyor project settings. Because of this it doesn't stop building stale commits promptly and takes longer to get to the current ones.

Copy link
Member

Choose a reason for hiding this comment

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

This'll actually want to drop the lock ASAP instead of holding it over the entire call to rustdoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, right 😳

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you put it like this then? It's a lot on one line and not very obvious that the lock will get released.

        if let Some(output) = build_state.outputs.lock().unwrap().get(&key) {
            for cfg in output.cfgs.iter() {
                rustdoc.arg("--cfg").arg(cfg);
            }
        }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the alternative is putting everything into an explicit block, not very nice looking either. Or explicit drop perhaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or

        if let Ok(outputs) = build_state.outputs.lock() {
            if let Some(output) = outputs.get(&key) {
                for cfg in output.cfgs.iter() {
                    rustdoc.arg("--cfg").arg(cfg);
                }
            }
        }
        /*
        else {
            return_an_error?
        }
        */

@alexcrichton
Copy link
Member

Whoa, I had no idea that option existed! I've turned it on now

@gkoz
Copy link
Contributor Author

gkoz commented Oct 11, 2015

I went with the more concise version giving the lock up after iterating over cfgs. At least I believe its lifetime will now be limited to the if let block.

@alexcrichton
Copy link
Member

@bors: r+ 807da6b

Thanks!

@bors
Copy link
Contributor

bors commented Oct 12, 2015

⌛ Testing commit 807da6b with merge 1dc1865...

bors added a commit that referenced this pull request Oct 12, 2015
@bors
Copy link
Contributor

bors commented Oct 12, 2015

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.

5 participants