Change FindDependencies to optionally follow symlinks

[thequailman]

I am using the FindDependencies function in this library to gather jsonnet files for future rendering. The current implementation of getAbsPath follows symlinks, which makes it very hard to determine later on what the file path should be when using a custom Importer. This change adds a flag to getAbsPath to make the symlink resolution optional. jsonnet-deps will still follow symlinks/there should be no behavior change for users.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[thequailman]

This should be ready to merge, if possible. We're currently maintaining a fork with this functionality and would love to remove the replace line in our go.mod.

[johnbartholomew]

Hello, and thank you for the contribution! Sorry it has been ignored for so long (over 2 years now...)

I think this is a breaking change for the go package, because it changes the parameters for a public function. Strictly speaking this is ok because there's no v1 yet, but it would still be nice not to change the API.

Is this still a use-case that you need? Are you still maintaining your own fork for this?

The current implementation of getAbsPath follows symlinks, which makes it very hard to determine later on what the file path should be when using a custom Importer.

In the current code, the call to filepath.EvalSymlinks only happens when the importer is a FileImporter, so if you're using a custom Importer then none of that should happen. So this suggests you're using the FileImporter when looking for dependencies, and then using a different importer when you actually evaluate the inputs - is that right?

I think the use of filepath.EvalSymlinks was probably included originally to try to ensure the same input file is not processed at multiple paths if it appears under different names. As an alternative to removing the EvalSymlinks call we could keep the "original" untranslated path in the deps map (so it is keyed by canonical path but holds the original path), and then we can report the untranslated path while retaining the same deduplication behaviour.

[thequailman]

Hello:

Is this still a use-case that you need? Are you still maintaining your own fork for this?

Yes, we still need this functionality. All of our products use Jsonnet, but one in particular, Etcha, builds/packages artifacts from various Jsonnet files. Avoiding the symlink resolution ensures we can properly walk various files and generate a tree of files to be packaged/imported. If we have to use symlink resolution, we end up importing/bundling the same file multiple times. We could work around this with hashing or dedupe algorithm, but just avoiding symlink resolution is far easier for us.

So this suggests you're using the FileImporter when looking for dependencies, and then using a different importer when you actually evaluate the inputs - is that right?

That's right, we basically do FindDependencies, gather the files, determine entry points, and store them in a map[<relative file path>]contents for bundling.

As an alternative to removing the EvalSymlinks call we could keep the "original" untranslated path in the deps map (so it is keyed by canonical path but holds the original path), and then we can report the untranslated path while retaining the same deduplication behaviour.

This could work maybe, but I'm not sure how we'd get this from FindDependencies. It returns a []string with the filepaths.

[johnbartholomew]

Trying to understand this correctly. So, hypothetically if you have a directory structure like this:

$ tree -l
.
├── entry.jsonnet
├── one.jsonnet -> real.jsonnet
├── real.jsonnet
└── two.jsonnet -> real.jsonnet
$ cat entry.jsonnet 
{
	x: import "one.jsonnet",
	y: import "two.jsonnet",
}
$ cat real.jsonnet 
{
	text: "hello",
}

You want output like this?

$ jsonnet-deps entry.jsonnet
/.../example/one.jsonnet
/.../example/two.jsonnet
$ jsonnet entry.jsonnet 
{
   "x": {
      "text": "hello"
   },
   "y": {
      "text": "hello"
   }
}

And then when you bundle the files you record the symlinks in your bundle (and presumably also follow them yourself to include the referenced files themselves in the bundle)?

Whereas right now without your modification you get:

$ jsonnet-deps entry.jsonnet
/.../example/real.jsonnet

Which is no use to you because although that represents the correct underlying data, entry.jsonnet doesn't actually reference real.jsonnet directly so including it in your bundle without the symlinks doesn't work when you later try to evaluate with only the bundled files.

[thequailman]

Exactly, appreciate you taking the time to understand the use case here. I'm fine using a different function from this library, it's a couple line change in our internal jsonnet library that abstracts this one. I could even change this PR to add FindDependenciesRelative instead that just doesn't follow symlinks so it doesn't break the API. Whatever works for you folks, I know my use case is a little niche here so appreciate however it ends up being included.

[johnbartholomew]

Ok, I think this makes sense. Although it might be overkill for a single flag I would prefer using the functional options pattern for this, so the method becomes:

type findDepsConfig struct {
    canonicalPaths bool
}

type FindDepsOption func(c *findDepsConfig)

// WithCanonicalPaths configures whether FindDependencies will canonicalize paths with filepath.EvalSymlinks. Each imported path will be reported only once as its canonicalized form, even if it is imported multiple times under different source names.
func WithCanonicalPaths(canonicalize bool) FindDepsOption {
    return func (c *findDepsConfig) { c.canonicalPaths = canonicalize }
}

func (vm *VM) FindDependencies(importedFrom string, importedPaths []string, opts ...FindDepsOption) ([]string, error) {
    config := findDepsConfig{
        canonicalPaths: true,
    }
    for _, f := range opts {
        f(&config)
    }

    // ...
}

// Caller can use
func someCallerCode() {
    vm.FindDependencies(from, paths, jsonnet.WithCanonicalPaths(false))
}

Not everyone likes the pattern, but the specific advantages I see here are:

• Although it technically is still an API-breaking change, normal usage should still compile without changes.
• Avoids needing to invent an appropriate name for an alternate method (still need to name the option
• Avoids using a plain bool flag parameter, as those are not nicely readable without remembering or looking up the documentation for the FindDependencies method to understand what the flag is (it's not obvious from the name FindDependencies what a bool flag would mean)

We can bikeshed about whether the option should be called WithFollowSymlinks, or WithCanonicalPaths or something else, if you have a strong opinion about it. I mildly prefer WithCanonicalPaths as it gets closer to describing the output rather than the mechanism, but naming is hard so I can be convinced in a different direction.

[thequailman]

@johnbartholomew sounds good to me, I pushed a new commit that should address all of your feedback.

EDIT: Sorry for the flurry of force pushes, my commit signing was messed up and made the CLA thing hate me.

[coveralls]

coverage: 44.314% (-0.04%) from 44.357%
when pulling 264982b on candiddev:optional-symlinks
into f4a8f75 on google:master.

[johnbartholomew]

Looks good. Could you run go fmt ./vm.go and amend the last commit?

[thequailman]

Should be all set now

[thequailman]

Thank you! Any idea when there will be a new release cut? I can point to this last commit in my go.mod for now.

[johnbartholomew]

I'm hoping to cut a new release in early March, but unfortunately I can't make a strong time commitment.