Switch to symbol RecordSymbol

[mosuem]

package:meta switched away from RecordMetadata in favor of the argumentless RecordUse, so introduce a custom annotation here which can be used for filtering information in the Dart link hook later on.

Related to dart-lang/i18n#941

These manual changes must be upstreamed into the Diplomat generator.

[Manishearth]

cc @robertbastian for upstreaming

[robertbastian]

The diplomat and gn test failures can be ignored, however test-dart should pass:

Failed to load "test/icu_test.dart":
  lib/src/lib.g.dart:802:23: Error: Undefined name 'mustBeConst'.
    const RecordSymbol(@mustBeConst this.symbol);
                        ^^^^^^^^^^^
  lib/src/lib.g.dart:802:23: Error: This can't be used as an annotation; an annotation should be a reference to a compile-time constant variable, or a call to a constant constructor.
    const RecordSymbol(@mustBeConst this.symbol);
                        ^
  package:test_core/src/runner/vm/platform.dart 242:7   VMPlatform._compileToKernel
  ===== asynchronous gap ===========================
  package:test_core/src/runner/vm/platform.dart 220:13  VMPlatform._spawnIsolate
  ===== asynchronous gap ===========================
  package:test_core/src/runner/vm/platform.dart 75:19   VMPlatform.load
  ===== asynchronous gap ===========================
  package:test_core/src/runner/loader.dart 219:27       Loader.loadFile.<fn>
  ===== asynchronous gap ===========================
  package:test_core/src/runner/load_suite.dart 97:19    new LoadSuite.<fn>.<fn>

[mosuem]

non_constant_identifier_names triggers on many method names unfortunately. The analysis options could just be written manually once, they don't have to be generated, same as the pubspec.

[robertbastian]

Which ones in particular? All FFI method names should have an // ignore.

The motivation here is that the output of Diplomat doesn't produce dart analyze errors. Yes the errors can be suppressed by a once manually written config, but that's not the point.

[robertbastian]

Suggested change

	sdk: ^3.7.0
	sdk: '>=3.7.0 <3.8.0'

[mosuem]

That would be a very tight constraint, and require more frequent updating. I would suggest >=3.7.0 <4.0.0 if you are worried about breaking changes.

[robertbastian]

yes it is tight, because it's an experimental feature. it broke between 3.4 and 3.7, it will break between 3.7 and 3.12

[mosuem]

And it will also break between 3.7.1 and 3.7.2 - so I don't see a reason to arbitrarily cut off here. And pinning it completely would make the usability even worse.

[robertbastian]

Ok then this should be =3.7.0, which is how ^3.4.0-204.0.dev behaved iiuc

[robertbastian]

I'm not comfortable setting a loose bound if we don't test against all possible versions in that bound. We could do that, but because this is not currently published, there's really no point.

[mosuem]

It's standard practice for most published Dart packages, and all published by the Dart team, to update the minimum version to the current stable as far as possible, and test against stable and dev versions. Users with older SDK versions can use older published versions.
For more experimental packages, you might require an even newer SDK, and only test against dev, as the expectation is that users trying out experimental packages will have the newest SDK versions. As this package is not yet published, you don't make any guarantees to it's usability, so any SDK bound and testing is fine. The bounds and testing procedures set now are only meant to support us in development. I find it easiest to set loose bounds, so that we don't have to go through the hassle of updating the bounds often while in development. As soon as the package is published, we can go to the established practice of keeping it up to date with the newest stable version.

[robertbastian]

We're not a Dart development team. We just want CI to work, and getting an error that you have the wrong Dart SDK is a million times better for us than getting a test failure in a language few people on the team have ever used.

[mosuem]

Then I would suggest deleting the testing from CI, so it always passes. This would be preferable to having a package which passes tests, but cannot be used. Would that be a compromise?

If this is not acceptable, then I will have to fork this package for further use, which would mean an additional workload I would rather not have.

[robertbastian]

But we are testing across changes to ICU4X FFI. We are just not testing across changes in the Dart compiler.

I thought you are using your own pubspec?

[mosuem]

Also tested against the intl4x PR at dart-lang/i18n#941, seems to work!

[robertbastian]

it's weird having the caller predict how the cargo output will be called

[mosuem]

I did not introduce that :D Is there a way around it?