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

cargo check --tests fails to compile macros in examples #5397

Closed
kornelski opened this issue Apr 21, 2018 · 4 comments
Closed

cargo check --tests fails to compile macros in examples #5397

kornelski opened this issue Apr 21, 2018 · 4 comments

Comments

@kornelski
Copy link
Contributor

Running cargo check --tests on https://github.com/nickel-org/nickel.rs unexpectedly can't find macros. However, cargo test --all compiles fine.

warning: unused #[macro_use] import
--> examples/https.rs:1:1
|
1 | #[macro_use] extern crate nickel;
| ^^^^^^^^^^^^
|
= note: #[warn(unused_imports)] on by default

error: cannot find macro middleware! in this scope
--> examples/integration_testing.rs:90:22
|
90 | server.post("/", middleware! { |req, res|
| ^^^^^^^^^^

error: cannot find macro lazy_static! in this scope
--> examples/integration_testing.rs:279:9
|
279 | lazy_static! {
| ^^^^^^^^^^^

  • cargo check --tests fails to compile examples
  • cargo check --examples does compile examples, but with warnings
  • cargo build --examples compiles examples without warnings
@matklad
Copy link
Member

matklad commented Apr 21, 2018

cc @ehuss it looks like something that would be fixed by #5384

@ehuss
Copy link
Contributor

ehuss commented Apr 21, 2018

Indeed, it should be fixed by #5384. Examples were being checked as tests (with --test) and thus was overriding your main function. In my changes, --tests will not include examples at all (you'll need to use --examples or --all-targets).

I think the warning is legitimate. At least for me, it appears on cargo build --examples. The issue is that the example has a cfg block for ssl which cuts off all usage of the macros. I think the fix is to add required-features = ["ssl"] to the https example in Cargo.toml instead of checking the feature in the code.

bors added a commit that referenced this issue Apr 27, 2018
Profile Overrides (RFC #2282 Part 1)

Profile Overrides (RFC #2282 Part 1)

WIP: Putting this up before I dig into writing tests, but should be mostly complete.  I also have a variety of questions below.

This implements the ability to override profiles for dependencies and build scripts.  This includes a general rework of how profiles work internally. Closes #5298.

Profile overrides are available with `profile-overrides` set in `cargo-features` in the manifest.

Part 2 is to implement profiles in config files (to be in a separate PR).

General overview of changes:

- `Profiles` moved to `core/profiles.rs`. All profile selection is centralized there.
- Removed Profile flags `test`, `doc`, `run_custom_build`, and `check`.
- Removed `Profile` from `Unit` and replaced it with two enums: `CompileMode` and `ProfileFor`.  This is the minimum information needed to compute profiles at a later stage.
- Also removed `rustc_args`/`rustdoc_args` from `Profile` and place them in `Context`.  This is currently not very elegant because it is a special case, but it works. An alternate solution I considered was to leave them in the `Profile` and add a special uber-override layer.  Let me know if you think it should change.
- Did some general cleanup in `generate_targets`.

## Misc Fixes
- `cargo check` now honors the `--release` flag.  Fixes #5218.
- `cargo build --test` will set `panic` correctly for dependences. Fixes #5369.
- `cargo check --tests` will no longer include bins twice (once as a normal check, once as a `--test` check).  It only does `--test` check now.
    - Similarly, `cargo check --test name` no longer implicitly checks bins.
- Examples are no longer considered a "test".  (See #5397). Consequences:
    - `cargo test` will continue to build examples as a regular build (no change).
    - `cargo test --tests` will no longer build examples at all.
    - `cargo test --all-targets` will no longer build examples as tests, but instead build them as a regular build (now matches `cargo test` behavior).
    - `cargo check --all-targets` will no longer check examples twice (once as
      normal, once as `--test`).  It now only checks it once as a normal
      target.

## Questions
- Thumbs up/down on the general approach?
- The method to detect if a package is a member of a workspace should probably be redone.  I'm uncertain of the best approach.  Maybe `Workspace.members` could be a set?
- `Hash` and `PartialEq` are implemented manually for `Profile` only to avoid matching on the `name` field.  The `name` field is only there for debug purposes. Is it worth it to keep `name`?  Maybe useful for future use (like #4140)?
- I'm unhappy with the `Finished` line summary that displays `[unoptimized + debuginfo]`.  It doesn't actually show what was compiled.  Currently it just picks the base "dev" or "release" profile.  I'm not sure what a good solution is (to be accurate it would need to potentially display a list of different options).  Is it ok?  (See also #4140 for the wrong profile name being printed.)
- Build-dependencies use different profiles based on whether or not `--release` flag is given.  This means that if you want build-dependencies to always use a specific set of settings, you have to specify both `[profile.dev.build_override]` and `[profile.release.build_override]`.  Is that reasonable (for now)?  I've noticed some issues (like #1774, #2234, #2424) discussing having more control over how build-dependencies are handled.
- `build --bench xxx` or `--benches` builds dependencies with dev profile, which may be surprising.  `--release` does the correct thing.  Perhaps print a warning when using `cargo build` that builds benchmark deps in dev mode?
- Should it warn/error if you have an override for a package that does not exist?
- Should it warn/error if you attempt to set `panic` on the `test` or `bench` profile?

## TODO
- I have a long list of tests to add.
- Address a few "TODO" comments left behind.
@ehuss
Copy link
Contributor

ehuss commented May 6, 2018

I investigated this some more. nickel.rs has a special set of integration tests for their examples. In particular, the one that errors out (integration_testing) uses include! to pull in the example into their testing framework.

I think this can be closed. With #5464 things should work better. cargo check --tests no longer checks examples directly, so it will no longer generate an error. The warning is legitimate (it looks like they removed SSL support since it was dropped from hyper, but didn't remove the https example which requires the ssl feature which no longer exists!).

@kornelski
Copy link
Contributor Author

Thank you for checking this.

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

No branches or pull requests

3 participants