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

incompatible_no_attr_license: Remove attr.license() #6420

Closed
laurentlb opened this issue Oct 17, 2018 · 8 comments

Comments

Projects
None yet
4 participants
@laurentlb
Copy link
Contributor

commented Oct 17, 2018

It's unclear how to use attr.license and I believe it almost never used. We should consider removing this function from the API.

Related discussion: https://groups.google.com/forum/#!topic/bazel-discuss/reYf1v_-_zU

cc @c-parsons @gregestren

@gregestren

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

Seconded. I think this should be innocuous no matter what we decide to do with licenses.

cc @aiuto

@aiuto

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2018

Agreed.

bazel-io pushed a commit that referenced this issue Oct 26, 2018

New flag --incompatible_no_attr_license to disable `attr.license` fun…
…ction

#6420

RELNOTES:
  The function `attr.license` is deprecated and will be removed.
  It can be disabled now with `--incompatible_no_attr_license`.
PiperOrigin-RevId: 218910996

@aiuto aiuto self-assigned this Nov 7, 2018

@aiuto

This comment has been minimized.

Copy link
Contributor

commented Nov 7, 2018

I'm prototyping a replacement for license(). While doing that, I'm working with the people who care about compliance within Google to define requirements for a checking scheme that can be implemented in Starlark. What I want to see is that the order of events is

  1. come up with a useful license gathering/enforcement rule set
  2. open a migration window for Google and external users to convert
  3. delete license() simulataneously in both Bazel and Blaze.

If possible, I want to avoid pulling the license code out of Bazel, but leaving it in Blaze, then cutting it out of Blaze a few months later. That just adds work with no use benefit.

@laurentlb

This comment has been minimized.

Copy link
Contributor Author

commented Nov 7, 2018

Sounds good. Can we document this on the website?

@laurentlb laurentlb changed the title Remove attr.license() --incompatible_no_attr_license: Remove attr.license() Nov 26, 2018

@werkt

This comment has been minimized.

Copy link
Contributor

commented Dec 31, 2018

I recently ran into a seemingly unavoidable use of attr.license(): creating a rule that is invoked under a third_party workspace path will implicitly invoke the rule with a licenses parameter. That licences attribute must be declared with type attr.license().

Leaving the attribute out of the rule attribute set yields an error:

ERROR: $workspace_root/third_party/example/BUILD:3:1: //third_party/example:example: no such attribute 'licenses' in 'rule_without_licenses' rule

Supplying any type other than attr.license() yields a bazel parse error (for all offending targets in the package) for paths under the third_party path:

ERROR: $workspace_root/third_party/example/BUILD:3:1: third-party rule '//third_party/example:example' lacks a license declaration with one of the following types: notice, reciprocal, permissive, restricted, unencumbered, by_exception_only

Supplying any type other than attr.license() yields a bazel crash for paths outside of the third_party path which use the rule:

Caused by: java.lang.IllegalArgumentException: wrong type for attribute "licenses" in rule_with_label_list_licenses rule //src/package:example
	at com.google.devtools.build.lib.packages.AbstractAttributeMapper.get(AbstractAttributeMapper.java:78)
	at com.google.devtools.build.lib.packages.AggregatingAttributeMapper.visitAttribute(AggregatingAttributeMapper.java:278)
	at com.google.devtools.build.lib.packages.AggregatingAttributeMapper.checkForDuplicateLabels(AggregatingAttributeMapper.java:142)
	at com.google.devtools.build.lib.packages.RuleClass.checkForDuplicateLabels(RuleClass.java:2108)
	at com.google.devtools.build.lib.packages.RuleClass.checkForDuplicateLabels(RuleClass.java:2073)
	at com.google.devtools.build.lib.packages.RuleClass.createRule(RuleClass.java:1817)
	at com.google.devtools.build.lib.packages.RuleFactory.createRule(RuleFactory.java:131)
	at com.google.devtools.build.lib.packages.RuleFactory.createAndAddRule(RuleFactory.java:177)
	at com.google.devtools.build.lib.packages.RuleFactory.createAndAddRule(RuleFactory.java:218)
	at com.google.devtools.build.lib.analysis.skylark.SkylarkRuleClassFunctions$SkylarkRuleFunction.call(SkylarkRuleClassFunctions.java:629)
	at com.google.devtools.build.lib.syntax.BaseFunction.callWithArgArray(BaseFunction.java:475)
	at com.google.devtools.build.lib.syntax.BaseFunction.call(BaseFunction.java:437)
	at com.google.devtools.build.lib.syntax.FuncallExpression.callFunction(FuncallExpression.java:981)
	at com.google.devtools.build.lib.syntax.FuncallExpression.doEval(FuncallExpression.java:893)
	at com.google.devtools.build.lib.syntax.Expression.eval(Expression.java:69)
	at com.google.devtools.build.lib.syntax.Eval.execDispatch(Eval.java:200)
	at com.google.devtools.build.lib.syntax.Eval.exec(Eval.java:182)
	at com.google.devtools.build.lib.syntax.BuildFileAST.execTopLevelStatement(BuildFileAST.java:230)
	at com.google.devtools.build.lib.syntax.BuildFileAST.exec(BuildFileAST.java:203)
	at com.google.devtools.build.lib.packages.PackageFactory.evaluateBuildFile(PackageFactory.java:1664)
	at com.google.devtools.build.lib.packages.PackageFactory.createPackageFromAst(PackageFactory.java:1293)
	at com.google.devtools.build.lib.skyframe.PackageFunction.loadPackage(PackageFunction.java:1210)
	at com.google.devtools.build.lib.skyframe.PackageFunction.compute(PackageFunction.java:444)
	at com.google.devtools.build.skyframe.AbstractParallelEvaluator$Evaluate.run(AbstractParallelEvaluator.java:422)
	... 4 more
Caused by: java.lang.ClassCastException: com.google.devtools.build.lib.packages.License cannot be cast to java.base/java.util.List
	at com.google.devtools.build.lib.syntax.Type$ListType.cast(Type.java:538)
	at com.google.devtools.build.lib.syntax.Type$ListType.cast(Type.java:521)

Do you have any recommendations for this scenario? Otherwise we should remove this from the incompatible grouping and into experimental - this is holding up my ability to be all_compatible_changes ready...

@gregestren

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2019

Would the suggestion in #3639 (comment) help?

Otherwise, perhaps make the third_party rule an alias that points out of third_party?

Ultimately I think this needs to be a Bazel cleanup. And under no circumstances should Bazel be crashing, regardless of the semantics.

@laurentlb laurentlb changed the title --incompatible_no_attr_license: Remove attr.license() incompatible_no_attr_license: Remove attr.license() Jan 10, 2019

@gregestren

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

Following up on my last comment, toggling --check_third_party_targets_have_licenses should fix @werkt 's issue. That flag will be available in upcoming Bazel release 0.23.

@gregestren

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2019

Never mind: my last comment was wrong. No flag fixes that crash. But I did find the root cause and have a PR which will fix it decisively for 0.24.

@bazel-io bazel-io closed this in 82281ad Feb 27, 2019

@gregestren gregestren reopened this Feb 27, 2019

@bazel-io bazel-io closed this in ce4b73e Mar 27, 2019

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Don't crash when Starlark rules declare attributes named "licenses".
The specific use case that triggers this is writing a rule with
a non-LICENSE-typed attribute named "licenses", then creating
a BUILD instance that doesn't explicitly set its value. See the test
for an example.

Fixes bazelbuild#7194.  Also fixes bazelbuild#6420 (comment),
which blocks the --incompatible_no_attr_license migration.

RELNOTES: Starlark rules can safely declare attributes named "licenses"
PiperOrigin-RevId: 235919628

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Enable --incompatible_no_attr_license by default
Fixes bazelbuild#6420

bazelbuild#7444

RELNOTES: --incompatible_no_attr_license is enabled by default
PiperOrigin-RevId: 240617932

irengrig added a commit to irengrig/bazel that referenced this issue May 3, 2019

Enable --incompatible_no_attr_license by default
Fixes bazelbuild#6420

bazelbuild#7444

RELNOTES: --incompatible_no_attr_license is enabled by default
PiperOrigin-RevId: 240617932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.