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

Stick down some versions so it builds #2

Merged
merged 1 commit into from Jul 5, 2020
Merged

Conversation

lf-
Copy link
Contributor

@lf- lf- commented Jul 5, 2020

This is not really "fixing" it entirely since there are still mysterious build failures on rustc 1.44-nightly 2020-04-09, only with cargo install --path ., but not with cargo build. Regardless, this PR causes it to sometimes build.

dev/srv - [fix-build●] » cargo install --path .
  Installing srv v0.1.0 (/home/lf/dev/srv)
    Updating crates.io index
    Updating git repository `https://github.com/gyscos/Cursive.git`
    Updating git repository `https://github.com/daboross/rust-screeps-api.git`
   Compiling srv v0.1.0 (/home/lf/dev/srv)
warning: unused imports: `cell::RefCell`, `rc::Rc`
 --> src/ui/console.rs:1:11
  |
1 | use std::{cell::RefCell, mem, rc::Rc};
  |           ^^^^^^^^^^^^^       ^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: unused imports: `CbSink`, `ColorStyle`, `Direction`, `EventResult`, `Event`, `Key`, `MouseButton`, `MouseEvent`, `Orientation`, `Printer`, `Vec2`, `XY`, `menu::MenuTr
ee`
  --> src/ui/console.rs:4:17
   |
4  |     direction::{Direction, Orientation},
   |                 ^^^^^^^^^  ^^^^^^^^^^^
5  |     event::{Event, EventResult, Key, MouseButton, MouseEvent},
   |             ^^^^^  ^^^^^^^^^^^  ^^^  ^^^^^^^^^^^  ^^^^^^^^^^
6  |     menu::MenuTree,
   |     ^^^^^^^^^^^^^^
7  |     theme::{BaseColor, Color, ColorStyle},
   |                               ^^^^^^^^^^
...
11 |     CbSink, Cursive, Printer, Vec2, XY,
   |     ^^^^^^           ^^^^^^^  ^^^^  ^^

error[E0220]: associated type `SinkError` not found for `futures_sink::Sink<websocket::message::OwnedMessage>`
   --> src/net.rs:252:28
    |
252 |     Si: Sink<OwnedMessage, SinkError = Error> + Unpin,
    |                            ^^^^^^^^^ associated type `SinkError` not found

error: aborting due to previous error

For more information about this error, try `rustc --explain E0220`.
error: failed to compile `srv v0.1.0 (/home/lf/dev/srv)`, intermediate artifacts can be found at `/home/lf/dev/srv/target`

Caused by:
  could not compile `srv`.

To learn more, run the command again with --verbose.

@daboross
Copy link
Owner

daboross commented Jul 5, 2020

Thanks!

I haven't maintained this project in a while, but I wouldn't be against getting it into a working state.

Copy link
Owner

@daboross daboross left a comment

Choose a reason for hiding this comment

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

Here are a few comments! I think the only thing I'd really object to here is adding the tag to screeps-api, though, if we can avoid that. Thanks for submitting this, though!

default-features = false
features = ["crossterm-backend"]
git = "https://github.com/gyscos/Cursive.git"
tag = "v0.13.0"
Copy link
Owner

Choose a reason for hiding this comment

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

If we're limiting it to this tag, how about just removing the optional git version altogether? I think the only reason it was here was to avoid bugs in 0.12.1-alpha.0 that were fixed but not released.

I'd keep the screeps-api git version as that crate is developed concurrently with this one, but this one should go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to bring it back to the state where it definitely built. I'm not sure if this limitation is required actually. It seems like the git source for cargo just compares the version in HEAD's Cargo.toml to the requested version and fails if it doesn't match, which was causing a build failure. Since I wanted to break one thing at a time I just put the tag here.

@@ -13,7 +13,7 @@ err-ctx = "0.2"
fern = "0.5"
hyper = "0.12"
hyper-tls = "0.3"
log = "0.4"
log = "^0.4.8"
Copy link
Owner

Choose a reason for hiding this comment

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

As I understand it, this line should be equivalent to the old one? Or at least, should be equivalent after running cargo update - all this does is increase the minimum version, which I don't think we should need to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolution conflict with another package that wanted 0.4.8. I have absolutely no idea why that happened though since I agree they should be compatible.

Copy link
Owner

Choose a reason for hiding this comment

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

I guess that make some sort of sense...? Cargo tries to keep multiple crates using similar versions if it can, but also won't update things implicitly. If you remove this line, does running cargo update again after doing that fix the conflict or does it still complain?

git = "https://github.com/daboross/rust-screeps-api.git"
tag = "screeps-api-0.6.0"
Copy link
Owner

Choose a reason for hiding this comment

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

If there are things in screeps-api which currently don't build, I think we should fix them there rather than avoiding the update here.

In particular, we're going to need an updated screeps-api to fix #1.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, just built locally and I see why you did this. I... don't entirely understand how the screeps-api-0.6.0 tag would work, though, when we're still listing the version dependency as 0.5?

I think long-term this tag should be removed, but if for now it fixes the build I'm OK with merging it until we update to the later screeps-api version 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually doesn't build with tag 0.5 either! What I suspect happened was that since these were developed concurrently, the git HEAD that was supposedly version 0.5 was actually a dev version of 0.6. Which is why I bumped it, because it builds if I do that. The version given to cargo can be increased though.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, that makes sense! I somehow forgot you'd also bumped the version when I was trying this out locally and the writing the second comment.

@daboross
Copy link
Owner

daboross commented Jul 5, 2020

With regards to the FutureSink error, if I had to guess I think it's probably our depending on an old version of futures-preview. I think if we set the version to something like version = "=0.3.0-alpha.whateverlastworked" it should fix it for now.

A better solution would be to fully migrate to async/await and futures 0.3, but I think that's more work.

@daboross
Copy link
Owner

daboross commented Jul 5, 2020

Thanks for explaining the reasoning behind the changes - I think this should be reasonable to merge now.

@daboross daboross merged commit 84b7a97 into daboross:master Jul 5, 2020
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.

None yet

2 participants