nextest-rs/nextest

Proposed design for including extra files in the archive

rukai opened this issue · 9 comments

rukai commented

Maintainer edit, tracking TODOs:

  • Add optional depth param, and limit depth by default to a reasonable value (we've chosen 8): #1411
  • Warn if a path isn't present (may allow this to be configured as required or skipped in the future) -- also warn if a depth = 0 is a directory, which is a similar situation: #1451
  • Check that path is well-formed: #1454
  • Add test for skipped file types (not file, dir or symlink): #1451
  • For errors, indicate why we're deciding to include a file in the archive (using an enum): #1454
  • Write docs: f74d299

From reading the discussion in #423 it sounds like a feature like this is welcome.
But I wanted to make a proposal for a design before I set about implementing it.

I propose that we add the following configuration options to the config file and they will behave as documented.

# `nextest archive` automatically includes any build output required by a standard build.
# However sometimes extra non-standard files are required.
# "archive-include-within-target" specifies additional paths that will be included in the archive.
#
# Example: archive-include-within-target = ["application-data", "data-from-some-dependency/file.txt"]
# In this example:
# * the directory and its contents at "target/application-data" will be included in the archive.
# * the file "target/data-from-some-dependency/file.txt" will be included in the archive.
archive-include-within-target = []

# "archive-include-within-target-profile" provides an additional list of paths within the
# current cargo profile to include.
#
# Example: archive-include-within-target-profile = ["application-data", "data-from-some-dependency/file.txt"]
# In this example when compiling with the release profile:
# * the directory and its contents at "target/release/application-data" will be included in the archive
# * the file "target/release/data-from-some-dependency/file.txt" will be included in the archive.
archive-include-within-target-profile = []

I believe that these 2 config options should cover all possible use cases.

Alternatively we could have a single config option and support * globbing for profile specific paths. e.g. */application-data
But that would mean that both release and debug builds would be included.

This design intentionally does not intend to add CLI flags for this as it feels like a build configuration level problem and not something the user would want ad-hoc control over.

But @sunshowers did mention including CLI flags for it in #423 , so maybe there is a motivation I'm missing.

sunshowers commented

Thanks for the proposal! I think at the time I wrote that comment in #423, our config system wasn't fleshed out to the degree that it is today. I completely agree that the right place to stick this is in configuration.

I think the way I'd do this is something like:

[profile.ci]
archive-include = [
    { path = "foo", relative-to = "target" },
    { path = "bar/*", relative-to = "target-profile" },
]

and write a small parser that replaces recognized names with the corresponding paths at runtime, erroring out if a match wasn't found.

  • Definitely ban absolute paths!
  • I'd definitely add globbing support as well -- I think users will expect to have it.
  • By default, I think specified lines should always match at least one path, erroring out if that condition doesn't hold. If it doesn't hold, then the way to do it is probably to let you say optional = true.

I'm enthusiastic about you (or someone else) doing it, so thanks again!

sunshowers commented

Note that in cross-compilation scenarios, target-profile can be more complex than just target/debug or similar.

rukai commented

I think your design makes sense since it seems more extensible and keeps all the include config in a single root level entry.

I'm not sure what you mean by "small parser that replaces recognized names with the corresponding paths at runtime.".
Can you give some examples of names that the parser would recognize?

sunshowers commented

Sorry, that bit was left over from an earlier iteration of the comment I was typing -- ignore it.

  • 👍1
sunshowers commented

@rukai: finally got some time to finish this up! Will release tomorrow during working hours Pacific time.

  • 🎉1
sunshowers commented

Ah sadly I couldn't get it done today, was busy at work and also realized that we needed to make some fixups. All good to go tomorrow though.

sunshowers commented

Just released cargo-nextest 0.9.69, binaries will be out in 15-20 minutes.

sunshowers commented

OK, out in cargo-nextest 0.9.70. Yay!