search

Factual / drake

Data workflow tool, like a "Make for data"
Other
1.48k stars 110 forks source link

%include path not relative (as stated in docs) #204

Closed potash closed 6 years ago

potash commented 9 years ago

According to the docs when using the %include directive

The filename is relative to the location of the parent workflow file (not the master workflow file Drake was originally invoked with).

But that doesn't seem to be true (currently or in previous versions). E.g.

Drakefile:

%include dir/Drakefile1

dir/Drakefile1:

%include Drakefile2

and let dir/Drakefile2 be anything (e.g. empty). Then drake throws

java.io.FileNotFoundException: ./Drakefile2 (No such file or directory)

According to the docs it should have been looking for dir/Drakefile2, no? For what it's worth, I also think it makes more sense.

amalloy commented 9 years ago

I can see where it looks like someone meant to implement this but never actually did: in call-or-include-helper, the file-path state variable is getting set for the subordinate workflow file, but is never actually read. It would be simple enough to change this behavior to match the documentation, but I'm a little concerned that this will impact people who are depending on it working the way it currently works. Thoughts, @dirtyvagabond?

chen-factual commented 9 years ago

One way we can handle this is keep the current behavior for %include (which I've always understand to act as an inline), and add this behavior for the upcoming %context directive, which is intended to be the inclusion directive that takes on the CWD of the included file.

Right now this only applies to shell cmds and steps run, but it would be cohesive and common sensical to extend this CWD change behavior to nested includes within a %context-ed file.

On Wed, Oct 21, 2015 at 10:17 AM, Alan Malloy notifications@github.com wrote:

I can see where it looks like someone meant to implement this but never actually did: in call-or-include-helper https://github.com/Factual/drake/blob/develop/src/drake/parser.clj#L660, the file-path state variable is getting set for the subordinate workflow file, but is never actually read. It would be simple enough to change this behavior to match the documentation, but I'm a little concerned that this will impact people who are depending on it working the way it currently works. Thoughts, @dirtyvagabond https://github.com/dirtyvagabond?

— Reply to this email directly or view it on GitHub https://github.com/Factual/drake/issues/204#issuecomment-149966832.

Factual Confidential Information This email contains Factual confidential information and is only for use by the intended recipient under the terms of a non-disclosure agreement or other binding confidentiality terms. If you've gotten this email in error, please let the sender know of the mistake and delete the email immediately.

chen-factual commented 9 years ago

Since the %context directive has been merged, should I incorporate this nested include feature with that? I.e. when you %include a file, the included file's includes will reference the original directory, whereas when you %context a file, the included file's includes will reference the included file's directory.

potash commented 9 years ago

@chen-factual Could you add a description of %context to the google doc? I think it would be useful for me but I don't know exactly what it does. Thanks.

chen-factual commented 9 years ago

@potash Yeah good idea. I'll take care of that tomorrow.

chen-factual commented 9 years ago

@amalloy @dirtyvagabond I added %context documentation to the Drake Google doc (plus a table of contents link). Can you please review this?

If this is good and clear, then we can discuss the matter of getting this behavior of relative referencing nested includes in %context-ed sub-workflows only.

Current behavior of %includes within %context-ed files is the same as base behavior, the include path is relative to the parent.

dirtyvagabond commented 8 years ago

thanks @chen-factual and apologies for the slow response. LGTM. let's get @amalloy's blessing as well... Alan, this ok with you?

chen-factual commented 8 years ago

Did you intend to assign to Alan instead of me?

dirtyvagabond commented 8 years ago

intended to assign to you @chen-factual . you're the owner here ... like it or not! ;-)

chen-factual commented 8 years ago

Ah I see. So the edit i made to the Drake doc is still listed as a "recommendation". I assume you're the owner, can you push that through?

I'll start working on making extending %context path behavior to %include paths sometime this week.

dirtyvagabond commented 8 years ago

done. thanks @chen-factual

chen-factual commented 8 years ago

I'll start working on making extending %context path behavior to %include paths sometime this week.

So one week became 4 months, but had some down time today to get this done. PR #211 submitted.