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

Ignore the benchmarks #411

Closed
wants to merge 2 commits into from

Conversation

dns2utf8
Copy link

So it can be published on crates.io

cc: @GuillaumeGomez

@dns2utf8
Copy link
Author

Enables rust-lang/rust#65894

@marcusklaas
Copy link
Collaborator

It seems we can keep the benchmarks if we remove it from the Cargo exclusion list, which seems like a nicer solution. Could you make that change in this PR, then I'll merge it and publish to crates.io.

@dns2utf8
Copy link
Author

I can do that.
On the other hand, why does the released version need the benchmark code?

@marcusklaas
Copy link
Collaborator

marcusklaas commented Nov 11, 2019

Because the code commented out refers to benchmarks, but when they are excluded from the crate they cannot be found and it blocks the publication.

edit: Realized that you may have meant something different. Benchmark code needn't be in the published package indeed, but without the lines that are commented out by this PR it's not possible to run benchmarks at all. If there's a way to have benchmarks without publishing them, I'm all for it.

@dns2utf8
Copy link
Author

It seems that it is currently not possible to release a package without the benchmark code included.
Even if that code is not touched during build later.

@marcusklaas
Copy link
Collaborator

Yup, seems that way. The current 0.6.1 release was done by including the benchmark code.

Thanks for your PR. If we find a way to exclude the benchmarks in the future, then let's revisit the subject.

@dns2utf8
Copy link
Author

There seems to be a discussion going on about maybe wanting to use included benchmarking code for compiler testing, but I am not sire that is the best idea.
Closing this PR for now. Thank you

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