Skip to content

feat(crates): Add noDevDeps option#112

Merged
tonyo merged 7 commits intomasterfrom
feat/crates-no-dev-deps
Jun 18, 2020
Merged

feat(crates): Add noDevDeps option#112
tonyo merged 7 commits intomasterfrom
feat/crates-no-dev-deps

Conversation

@jan-auer
Copy link
Member

Cargo does not allow to publish crates with circular dependencies. In dev dependencies, they would theoretically be fine since they are not required to run or build the crate. We need them in the Rust SDK to provide proper doc comments in sub-crates.

This PR adds a new noDevDeps option, which uses cargo hack to strip dev dependencies before publishing.

},
},
additionalProperties: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, does this actually work?
Based on the comment above, I really doubt it.
But we can probably revisit configuration validation some time soon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure :) I only added it for completeness since I saw the others. Certainly doesn't hurt.

@jan-auer
Copy link
Member Author

This still needs tests.

@jan-auer jan-auer force-pushed the feat/crates-no-dev-deps branch from cd20279 to ba4471f Compare June 18, 2020 15:15
@jan-auer
Copy link
Member Author

@tonyo should be ready now (if the Docker image builds). Added tests, too.

Copy link
Contributor

@tonyo tonyo left a comment

Choose a reason for hiding this comment

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

📦

process.env.CRATES_IO_TOKEN = 'xxx';
const target = new CratesTarget({}, new NoneArtifactProvider());
const target = new CratesTarget(
{ noDevDeps: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be nice to add an interface for CratesTargetConfig (since it's not automatically generated now).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have an example for where to put and enforce this? I looked into the other targets, but they all receive an any config.

Copy link
Contributor

Choose a reason for hiding this comment

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

They do, but then we perform manual validation, e.g. https://github.com/getsentry/craft/blob/master/src/targets/npm.ts#L131-L158

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent, thanks. Added.

@jan-auer jan-auer force-pushed the feat/crates-no-dev-deps branch from e2c811e to 985ab87 Compare June 18, 2020 16:34
@jan-auer
Copy link
Member Author

Travis is not reporting status. Adding a very useful reformat commit to trigger.

@jan-auer
Copy link
Member Author

This is the passing PR build: https://travis-ci.org/github/getsentry/craft/builds/699788879
@tonyo mind if we merge?

@tonyo tonyo merged commit 4dee772 into master Jun 18, 2020
@tonyo tonyo deleted the feat/crates-no-dev-deps branch June 18, 2020 17:11
@tonyo
Copy link
Contributor

tonyo commented Jun 18, 2020

yow low

jan-auer added a commit that referenced this pull request Jun 18, 2020
* master: (23 commits)
  feat(crates): Add noDevDeps option (#112)
  fix: Write to cache in base artifact provider
  fix: Logger scopes for gcs and artifact providers
  build(ci): Have better defaults for CI environments (#110)
  fix(gha): Use single quotes for string literals (#108)
  ref(gha): Remove ENV inputs, add no-merge and keep-branch (#107)
  fix(docker): Fix CARGO_HOME and RUST_HOME since GHA changes HOME (#106)
  docs: Add missing CHANGELOG entry for cargo upgrade (#105)
  build(docker): Upgrade cargo to a recent version (#104)
  fix(gha): Remove no-merge and keep-branch temporarily
  fix(gha): Try to skip empty args using null
  fix(gha): Remove defaults on craft arguments
  fix(gha): Only pass publish args to publish
  feat(gha): Add GitHub Action for Craft (#103)
  docs: Fix `changelogPolicy` enum (#102)
  build(docker): Add a `craft` binary into the Docker image (#101)
  docs: Fix `artifactProvider` example (#100)
  release: 0.10.0
  meta: Update Changelog
  build(gcb): Add a public Docker image (#99)
  ...
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