#59 [WIP] support for optional features
Closed 2 years ago by zbyszek. Opened 5 years ago by zbyszek.
fedora-rust/ zbyszek/rust2rpm optional-features  into  master

file modified
+31 -5
@@ -83,6 +83,14 @@ 

      t = datetime.fromtimestamp(os.stat(path).st_mtime, timezone.utc)

      return t.astimezone().isoformat()

  

+ @contextlib.contextmanager

+ def remove_on_error(path):

+     try:

+         yield

+     except: # this is supposed to include ^C

+         os.unlink(path)

+         raise

+ 

  def local_toml(toml, version):

      if os.path.isdir(toml):

          toml = os.path.join(toml, "Cargo.toml")
@@ -110,7 +118,8 @@ 

          req = requests.get(url, stream=True)

          req.raise_for_status()

          total = int(req.headers["Content-Length"])

-         with open(cratef, "wb") as f:

+         with remove_on_error(cratef), \

+              open(cratef, "wb") as f:

              for chunk in tqdm.tqdm(req.iter_content(), "Downloading {}".format(cratef_base),

                                     total=total, unit="B", unit_scale=True):

                  f.write(chunk)
@@ -163,22 +172,29 @@ 

  def _is_path(path):

      return "/" in path or path in {".", ".."}

  

- def make_diff_metadata(crate, version, patch=False, store=False):

+ def make_diff_metadata(crate, version, patch=False, store=False, *, enabled_features, disabled_features):

      if _is_path(crate):

          # Only things that look like a paths are considered local arguments

          if crate.endswith(".crate"):

              cratef, crate, version = local_crate(crate, version)

          else:

+             if store:

+                 raise ValueError('--store-crate can only be used for a crate')

+ 

              toml, crate, version = local_toml(crate, version)

              diff = make_patch(toml, enabled=patch, tmpfile=True)

-             metadata = Metadata.from_file(toml)

+             metadata = Metadata.from_file(toml,

+                                           enabled_features=enabled_features,

+                                           disabled_features=disabled_features)

              return metadata.name, diff, metadata

      else:

          cratef, crate, version = download(crate, version)

  

      with toml_from_crate(cratef, crate, version) as toml:

          diff = make_patch(toml, enabled=patch)

-         metadata = Metadata.from_file(toml)

+         metadata = Metadata.from_file(toml,

+                                       enabled_features=enabled_features,

+                                       disabled_features=disabled_features)

      if store:

          shutil.copy2(cratef, os.path.join(os.getcwd(), f"{crate}-{version}.crate"))

      return crate, diff, metadata
@@ -197,6 +213,10 @@ 

                          help="Do initial patching of Cargo.toml")

      parser.add_argument("-s", "--store-crate", action="store_true",

                          help="Store crate in current directory")

+     parser.add_argument("--with", nargs="*", dest="_with",

+                         help="Enable the specified features (default: all)")

+     parser.add_argument("--without", nargs="*",

+                         help="Disable the specified features")

      parser.add_argument("crate", help="crates.io name\n"

                                        "path/to/local.crate\n"

                                        "path/to/project/",
@@ -211,9 +231,15 @@ 

      if args.crate is None:

          parser.error('required crate/path argument missing')

  

+     confused = set(args._with or ()) & set(args.without or ())

+     if confused:

+         parser.error(f"Cannot both disable and enable features at the same time: {' '.join(confused)}")

+ 

      crate, diff, metadata = make_diff_metadata(args.crate, args.version,

                                                 patch=args.patch,

-                                                store=args.store_crate)

+                                                store=args.store_crate,

+                                                enabled_features=args._with,

+                                                disabled_features=args.without)

  

      template = JINJA_ENV.get_template("main.spec")

  

file modified
+18 -4
@@ -149,7 +149,7 @@ 

          self.test_requires = []

  

      @classmethod

-     def from_json(cls, metadata):

+     def from_json(cls, metadata, enabled_features=None, disabled_features=None):

          self = cls()

  

          md = metadata
@@ -195,14 +195,28 @@ 

                            file=sys.stderr)

                      continue

  

-             requires.append(Dependency(dep["name"], dep["req"], features=dep["features"]))

+             name = dep["name"]

+             optional = dep["optional"]

+             print(f'enabled={enabled_features} disabled={disabled_features} {name}')

+             if ((enabled_features is not None and name not in enabled_features and optional) or

+                 (disabled_features is not None and name in disabled_features)):

+ 

+                 if not optional:

+                     raise ValueError(f'Cannot disable non-optional feature "{name}"')

+ 

+                 print(f'Skipping disabled dependency "{name}".', file=sys.stderr)

+                 continue

+ 

+             requires.append(Dependency(name, dep["req"], features=dep["features"]))

  

          return self

  

      @classmethod

-     def from_file(cls, path):

+     def from_file(cls, path, enabled_features=None, disabled_features=None):

          do_decode = sys.version_info < (3, 6)

          metadata = subprocess.check_output(["cargo", "read-manifest",

                                              "--manifest-path={}".format(path)],

                                             universal_newlines=do_decode)

-         return cls.from_json(json.loads(metadata))

+         return cls.from_json(json.loads(metadata),

+                              enabled_features=enabled_features,

+                              disabled_features=disabled_features)

The first two commits are part of PR #58, please ignore them here.

The third commit adds ability to specify features to enable/disable on the rust2rpm command line. Unfortunately this gains us nothing, because cargo has no mechanism to ignore dependencies and requires all dependencies to be always present.

I think we should just wait for cargo to solve this. All distributions want/need this, so doing something like patching .toml files locally just doesn't seem worth the trouble. See upstream bug https://github.com/rust-lang/cargo/issues/4544.

@zbyszek I have been working on a small CLI tool (cargoman) that "normalizes" targets and dependencies (by dropping non-linux targets and their dependencies), which would work really well with this PR (especially once I implement dropping unused optional dependencies) ... are you still interested in getting this into rust2rpm?

(I eventually hope to be able to integrate the tool with our Rust macros, it should allow us to drop almost all Cargo.toml patches in fedora.)

Yes, I think this is still relevant.

I really think cargo should allow some dependencies to be ignored. But there seems to be no interest in this upstream, so I think a tool in rust to manipulate the .toml file is a reasonable work-around.

So... what plan of action would you suggest?

Yeah, I looked at the upstream issue and nothing seems to have happened in two years ...

I'll try implementing the "remove features and their associated optional dependencies from Cargo.toml" feature in cargoman later today. Once that's done, I can start putting things together.

I've never written RPM macros before, so there's some learning involved :)

But I imagine something like this:

  • this PR: generate new spec with "rust2rpm foo --without unwanted-feature" to make sure the +unwanted-feature-devel subpackages don't get generated
  • add %cargo_remove_feature unwanted-feature macro for use in in %prep to modify the Cargo.toml file accordingly

I have already implemented "target normalization" support, so we could have something like a %cargo_normalize_targets (or even automatically do that as part of %cargo_prep?) so there's no need to manually patch Cargo.toml files for either of those use cases.

(cargoman also has support for overriding dependency versions, so that will no longer need patches either, instead we could use something like %cargo_set_dependency foo "^1.0")

What do you think?

I would prefer having to specify the unwanted features twice. One option would be to tweak rust2rpm to generate appropriate %cargo_remove_feature calls in the spec file. But I think it would be nicer to store this information in an external file:

  • store unwanted-feature and other unwanted features in a config file foo.features.ini (format TBD)
  • call rust2rpm foo --features foo.features.ini
    and this would insert an appropriate call to %cargo_remove_features foo.features.ini

By separating the config into a file we make things more declarative and allow rust2rpm to be re-run more easily.

You mean integrating it into .rust2rpm.conf files like this one?
https://src.fedoraproject.org/rpms/rust-clang-sys/blob/master/f/.rust2rpm.conf

I find the syntax abhorrent but I think we could manage to do that.

Or I could finally rewrite rust2rpm in Rust and use TOML as config file format ... :/

.rust2rpm.conf

Yeah, I think .rust2rpm.conf is the appropriate place. I didn't know about this file.

I find the syntax abhorrent but I think we could manage to do that.

If you have a better syntax in mind, we can change it. We have pretty much complete freedom here (as long as backwards compat is kept).

Or I could finally rewrite rust2rpm in Rust and use TOML as config file format ... :/

We can use toml in Python... I don't think the config file language is the important part. Having the tooling in python makes is much easier to share bits between various language ecosystems. E.g. I would like to share rust2rpm.licensing. With a rust implementation, this wouldn't be so easy.

I implemented the .spec file generator part of "unwanted-features" here:
https://pagure.io/fedora-rust/rust2rpm/pull-request/117

This adds a "unwanted-features" key-value pair and the necessary changes to the .spec template. So when specifying a feature as "unwanted" in the config file, it will not get a +feature-devel subpackage generated for it.

If you want to reuse this value for your PR, to_list(distconf.get("unwanted-features")) gives you the list of "unwanted-features" that were specified in the config file, if any - and if none were specified, this call would return an empty list.

So, I've finished my implementation of the Cargo.toml mangling to remove features, now I only need to write the macros ...

I think having rust2rpm instert %cargo_remove_feature $feature macro calls in %prep automatically based on the values in .rust2rpm.conf would be the easiest solution now

So, for cases where the actual Cargo.toml file doesn't actually need to be patched (e.g. removing completely optional features or dependencies that are not part of the "default" feature set), rust2rpm 16 and the new "unwanted-features" config flag works fine, which covers almost all cases, I think.

This is 3 years old at this point… Even if there are some good ideas, it'll need to be done from scratch anyway. I'll close it to keep the pull request list tidy.

Pull-Request has been closed by zbyszek

2 years ago