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

Update dependencies: Use serde_json instead of rustc-serialize, update winapi, log #104

Merged
merged 4 commits into from
Mar 13, 2018
Merged

Conversation

messense
Copy link
Contributor

@messense messense commented Mar 3, 2018

rustc-serialize is deprecated.

@messense
Copy link
Contributor Author

messense commented Mar 3, 2018

It seems that Travis CI is disabled?

@@ -27,7 +27,7 @@ struct Diagnostic {
rendered: Option<String>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rendered field is unused, should I remove it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I never answered your question here. I'm about to release 0.3.8 and wondering if this warning should be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess it occurs in rustc/compiletest as well?

Copy link
Contributor Author

@messense messense Mar 13, 2018

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm, ok thanks for checking that, I'll remove it and release 0.3.9 with that if the need arises

@laumann
Copy link
Collaborator

laumann commented Mar 3, 2018

Hi @messense Thanks for your PR!

Any chance you could make this PR against rustc/compiletest? This project tries to maintain (some level of) parity with that project, at least to the point of always building on nigthly. New changes in this project should preferably also be pushed back to rustc/compiletest, otherwise it's going to be harder to maintain this project when changes from rustc/compiletest have to be integrated.

It seems that Travis CI is disabled?

Yes, right now it's just AppVeyor, I haven't really seen the need for both.

@messense
Copy link
Contributor Author

messense commented Mar 3, 2018

I don't think it's easy to upstream this, according to rust-lang/rust#40527 (comment)

I think enable Travis CI is needed to make sure things like#[cfg(unix)](for example here) annotations actually work and don't break in the future. cc #97

@messense
Copy link
Contributor Author

messense commented Mar 3, 2018

For the maintainability issue, I think this patch is quite small thus should not get in the way of syncing upstream changes to here.

@laumann
Copy link
Collaborator

laumann commented Mar 5, 2018

I don't think it's easy to upstream this, according to rust-lang/rust#40527 (comment)

Then I'm reluctant to merge this here, as I'd like to keep the differences between the two projects as minimal as possible. If you look through the history, the most deviation is in lib.rs and other changes are predominantly adding things, not changing existing things.

And for the maintainability, every single change looks small, but this kind of thing snowballs quite easily and then becomes a bigger problem than it should be.

@SergioBenitez has mentioned a few times that this project ideally should be rewritten from scratch to address some of the usability issues that arise when compiletest is used outside of rustc. See #81 for an example. This kind of change belongs there in my opinion. But I'd @SergioBenitez's opinion as well - should we merge this?

@laumann
Copy link
Collaborator

laumann commented Mar 5, 2018

I think enable Travis CI is needed to make sure things like#[cfg(unix)](for example here) annotations actually work and don't break in the future. cc #97

Again, couldn't this be addressed by adding another target in AppVeyor to have it built on a un*x platform?

@messense
Copy link
Contributor Author

messense commented Mar 6, 2018

Again, couldn't this be addressed by adding another target in AppVeyor to have it built on a un*x platform?

Adding another un*x target on AppVeyor would be fine. Although I am not sure how to do that since AppVeyor is a Windows CI service and it will slow down build time since it only has 1 concurrent build.

@messense
Copy link
Contributor Author

messense commented Mar 7, 2018

rust-lang/rust#48798

It's happening!

@laumann
Copy link
Collaborator

laumann commented Mar 7, 2018

Yay! Awesome :-) I'll be much happier to merge this then.

@laumann laumann changed the title Use serde_json instead of rustc-serialize Update dependencies: Use serde_json instead of rustc-serialize, update winapi, log Mar 7, 2018
@laumann
Copy link
Collaborator

laumann commented Mar 13, 2018

Oh, rust-lang/rust#48798 was merged, I'll merge this here then.

@laumann laumann merged commit ebbbe28 into Manishearth:master Mar 13, 2018
@messense messense deleted the feature/serde branch March 13, 2018 06:09
@laumann
Copy link
Collaborator

laumann commented Mar 13, 2018

0.3.8 is out!

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