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

feature/legacy support #93

Merged
merged 9 commits into from
Feb 1, 2019
Merged

feature/legacy support #93

merged 9 commits into from
Feb 1, 2019

Conversation

rustysec
Copy link
Contributor

updated solution #91, simple feature toggle without any code modifications. i added a couple of test cases that show the error handling still works even with --no-default-features --feature=legacy-support enabled (all test cases are passing).

@rustysec
Copy link
Contributor Author

the travis failure gets cleaned up if you do a cargo clean between builds. let me see what i can do to make travis behave.

@punkstarman
Copy link
Collaborator

@rustysec, the caching problem was encountered on the master branch. I fixed it by manually clearing the cache on that branch. I believe that the problem happens when a new version of Rust is released.

I'd rather not remove caching entirely because it does speed up the CI considerably. I'd rather open a new issue to investigate this problem.

@rustysec
Copy link
Contributor Author

Gotcha, I will fix the config

@rustysec
Copy link
Contributor Author

stable is failing in travis, but beta and nightly are working. i'm building with stable on my machine(s) and all is well.

@punkstarman
Copy link
Collaborator

I wonder if the build failure is due to an issue on docmatic (assert-rs/docmatic#4)

@RReverser
Copy link
Owner

@punkstarman That does sound realistic. I wonder if we could just drop Nightly from the testing matrix - in my experience, it should be always backwards-compatible with stable Rust, and when it's not, it's an issue with Rust itself and not with the crate so testing is sort of pointless.

@punkstarman
Copy link
Collaborator

@RReverser the problem is on Stable not Nightly.

I don't mind removing Nightly from CI script. Yet, IMO the point of building with Nightly is to possibly anticipate rare occasions where backwards-compatibility will break and to help detect regressions in Nightly. Also, the current CI script ignores failed Nightly builds.

  allow_failures:
    - rust: nightly

@RReverser
Copy link
Owner

I see, I got confused by reading the linked docmatic issue. As far as I understand comments there, cleaning the cache for package itself works around this problem?

If so, maybe worth trying to cargo clean -p serde-xml-rs as part of Travis script (probably even as part of before_cache step)? This should still speed up most builds significantly, and is a more gentle / reliable option than the suggested rm -rf ....

@rustysec
Copy link
Contributor Author

I think that would work, Travis worked when I flipped caching off in the yaml. This solution would be better.

@punkstarman
Copy link
Collaborator

I'm game for trying cargo clean ... as a before_cache action.

@rustysec
Copy link
Contributor Author

the key seems to be removing the old cached lib from the build directory.

@rustysec
Copy link
Contributor Author

i thought cargo would do that, but a simple rm handles the problem instead

@RReverser
Copy link
Owner

That's weird, I'd indeed expect Cargo to handle that.

@RReverser
Copy link
Owner

Btw, I see you moved commands from before_cache - any specific reason?

@rustysec
Copy link
Contributor Author

it didn't work in either place, only when i added the rm did it build

@RReverser
Copy link
Owner

Yeah, but I suppose the difference was in rm so now cleanup can be moved back to before_cache?

@RReverser
Copy link
Owner

(I mean rm itself as well)

@rustysec
Copy link
Contributor Author

aha sorry about that

@rustysec
Copy link
Contributor Author

is there anything outstanding on this?

@RReverser
Copy link
Owner

I think it would be good to clean up commit history (to avoid lots of small cleanup commits, and also avoid them showing from two different authors) but otherwise LGTM, leaving to @punkstarman for final review.

@rustysec
Copy link
Contributor Author

i think a squash merge is a good idea

@punkstarman
Copy link
Collaborator

LGTM too.

Concerning clean up or no, I don't mind either way. I'll do a squash merge.

@rustysec, I'll also tag and release to crates.io soon.

@RReverser, for my personal continuous improvement, I'd be interested in understanding why cleanup is important to you.

@punkstarman punkstarman self-assigned this Feb 1, 2019
@punkstarman punkstarman merged commit b854e1a into RReverser:master Feb 1, 2019
@RReverser
Copy link
Owner

@punkstarman I believe clean, maintained commit history can serve a list of useful purposes while not being too hard to keep:

  1. When each commit is an atomic self-descriptive change, it's easy to treat the log itself as a CHANGELOG for those interested, rather than having to maintain one separately.
  2. When each commit individually is in a working condition, then, when a bug is found, you can do git bisect run ... to automatically find where it was introduced, and not stumbling on intermediate failing WIP commits.
  3. git blame as well as commit view become more clear to read and it's easier to see which changes were introduced together, which is often useful to understand why particular API or test exists.

There might be more, but these are the ones I can name off the top of my head.

punkstarman pushed a commit that referenced this pull request Jul 29, 2020
feature toggle for backtrace
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.

3 participants