Export command

[eode]

Summary

Includes quilt export and command.export(...)

Only handles FileNode objects, not TableNode objects (minimum for TF support).

Depends On

#536 Minor test improvements
#537 Build.py uncaught exception fix, other minor misc from export PR

[eode]

So, since it's been decided to permit absolute paths in the registy, people can now write files to arbitrary locations on the exporter's system. That's a pretty serious issue, so this branch probably isn't good to go until that issue is resolved. In the short term, I figure:

Either this:

X:\Project Data\FooBar> quilt export foo/bar .\
Error:  This package contains absolute references outside of X:\Project Data\FooBar:
C:\WindowsHelp\README
C:\WindowsHelp\ExampleData
C:\Windows\HelpPane.exe
C:\bootmgr

To export this package, use:
  quilt export foo/bar --allow-absolute

USING THE ABOVE COMMAND IS A SECURITY RISK!!!
Make sure writing to the listed paths is acceptable, and that the creator is trusted.

Or this:

X:\Project Data\FooBar> quilt export foo/bar .\
<export output>
Note:  This package contains files with absolute references to "C:\".
    They have been exported to the folder "C" in the export dir.

..substituting "\" with "UNC", ":" with "" on windows, and "" with "root" on Linux. ..or similar. I'm open to ideas.

But to merge without some change like that, I'd kinda need to have a firm "Go ahead, I'm taking responsibility for this one."

The simplest path is the second option: Re-root into the export dir. Notably, that would still allow people to build packages from other locations (including info from other places than the build dir when building a package), without requiring them to make NTFS Junctions, or links, or Linux symlinks. Referencing things outside the build dir via absolute paths is currently actively used.

[kevinemoore]

@eode I think we're conflating the need to specify source files using absolute paths and a specification of the destination of a file for export. IMHO it doesn't make sense to allow absolute paths for export, but that doesn't mean that users shouldn't be able to use absolute paths in build files.

[eode]

@kevinemoore I can definitely agree with that. My preference is to re-root absolute paths into the export dir (#2 above).

If an exporting user wants the exported files to go into their original location, they can export from that location, specify that location as the destination, or copy/move them to that location.

If someone wants to make their package prettier when exporting, we can add something like this as a feature:

contents:
  images:
    base:  "/abs/path"
    pellets: 
      # this would use "/abs/path/images/pellets.png" and export to "<export_dir>/images/pellets.png"
      file: "images/pellets.png"

But in any-case, re-homing absolute files into the export dir, as per option 2 above, or ignoring them completely is fine with me.

[quiltdata]

I think we're on the same page; I would adjust the syntax though. Make file: into a list or dictionary and specify the export mapping (for absolute paths) right there.

…

On Feb 17, 2018, at 14:20, eode ***@***.***> wrote: @kevinemoore I can definitely agree with that. My preference is to re-root absolute paths into the export dir (#2 above). If an exporting user wants the exported files to go into their original location, they can export from that location (or specify that location as the destination). If someone wants to make their package prettier when exporting, we can add something like this as a feature: contents: images: base: "/abs/path" pellets: # this would use "/abs/path/images/pellets.png" and export to "<export_dir>/images/pellets.png" file: "images/pellets.png" "yeardata/year01" But in any-case, re-homing absolute files into the export dir, as per option 2 above, or ignoring them completely is fine with me. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread. -- You received this message because you are subscribed to the Google Groups "Developers" group. To unsubscribe from this group and stop receiving emails from it, send an email to ***@***.*** To post to this group, send email to ***@***.*** To view this discussion on the web visit https://groups.google.com/a/quiltdata.io/d/msgid/dev/quiltdata/quilt/pull/385/c366472259%40github.com.

[kevinemoore]

My only remaining change requests are minor cleanup. Remove parameter description in comments (see below) and please remove the extra copies of command.py.

Just a comment, it's better to have the added helpers inside the export command, but I still don't think they're needed. Unless I'm missing something, I think you could grab the core nodes tree from the Package object and use the built-in helpers. If the helpers & iterators in that class don't do what's needed for this function, we should consider improving and/or expanding them.

[kevinemoore]

Remove comment about removed filter and mapper parameters.

[eode]

OK, I've got to set a max float size for Pandas CSV output, so I used 256 bit / 78 decimal.

Added roundtrip build/export test.

Now that the other PRs have gone though, this is a lot cleaner now, as well.

Note: Although this doesn't add to nodes.py since that's being phased out at some point, it does use nodes.py -- until such functionality is created for core.py nodes. It seems pointless to write the extra code that would be needed to use core.py, as that will need to be written separately, anyways.

Once the core.py API is complete and has useful features, we can switch to using that instead.

[eode]

As to using existing helpers -- they don't exist yet. :-) But, that was discussed earlier, this note is just for posterity.

This is ready for review again.

[kevinemoore]

Looks good, but I still see extra files that look like copies of command.py in the diffs.

[kevinemoore]

This looks good, but the diff still shows what looks like added files: compiler/quilt/tools/command_BACKUP_10243.py

[eode]

Whoops -- yup, was trying a new tool, and missed/accidentally included those.

[eode]

Merging -- Kevin's remaining issues have been addressed, and is ok with this now (per slack with Aneesh).