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

Include guards around os-specific string elements. #1271

Merged
merged 4 commits into from
May 25, 2018

Conversation

stevepentland
Copy link

@stevepentland stevepentland commented May 6, 2018

This seems to be something missed some time ago as the original annotation for this 'use' was only feature debug. However this broke builds using that feature on Windows for a while.

This change uses guards both for the feature and also wasm/windows specific targets/environments.
These guards are the same as are currently in use for Windows/wasm in parser.rs

closes #1270 and is done in response to Drops-of-Diamond/diamond_drops#59

fyi: @jamesray1


This change is Reviewable

This seems to be something missed some time ago as the original
annotation for this 'use' was only feature debug. However this
broke builds using that feature on Windows.

This change uses guards both for the feature and also wasm/windows
specific targets/environments.

closes clap-rs#1270 and is done in response to Drops-of-Diamond/diamond_drops#59
@mention-bot
Copy link

@stevepentland, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kbknapp to be a potential reviewer.

@stevepentland
Copy link
Author

stevepentland commented May 6, 2018

I cannot locally reproduce the 1.20.0 failure, but it seems to be coming out of the unicode-normalization crate that is a dependency of our dev depencency version-sync

Edit: After looking at the travis build history, it appears this failure started happening about 3 days ago

Copy link

@jamesray1 jamesray1 left a comment

Choose a reason for hiding this comment

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

LGTM! @ChosunOne

@ChosunOne
Copy link

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@jamesray1
Copy link

jamesray1 commented May 9, 2018

Obviously the build needs to pass for 1.20.0 before merging.

@stevepentland
Copy link
Author

stevepentland commented May 9, 2018

Obviously the build needs to pass for 1.20.0 before merging.

Yeah, but as I mentioned in my previous comment, the failure seems to be unrelated to this particular change and looks to have started about 3 days prior to this PR. The first time it failed was during this run.

@jamesray1
Copy link

OK yeah fair enough, all the errors are coming from unicode-normalization-0.1.6/src/tables.rs.

@stevepentland
Copy link
Author

stevepentland commented May 9, 2018

It seems @kbknapp has been pretty quiet for a bit so we'll just have to wait for him to get back and offer some guidance on fixing the issue. The last change that went into master was merging #1244 which was passing when it came in, so it seems like something external may be responsible.

@kbknapp
Copy link
Member

kbknapp commented May 22, 2018

Sorry all, I was out for a while with my day job. I'm taking a look at this and will merge shortly 😉

@kbknapp kbknapp merged commit e07b302 into clap-rs:master May 25, 2018
@jamesray1
Copy link

Thanks! Let me know when this is in a release so that I can update the version in cargo.toml.

kbknapp added a commit that referenced this pull request Jun 12, 2018
Include guards around os-specific string elements.
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.

Investigate windows wasm build issue
5 participants