Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: lockfiles #3231

Merged
merged 14 commits into from Nov 3, 2019

Conversation

@ry
Copy link
Collaborator

ry commented Oct 29, 2019

Fixes #200

Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
@ry ry changed the title First pass at lockfile support feat: lockfiles Oct 29, 2019
ry added 2 commits Oct 29, 2019
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 29, 2019

💯!

I wonder if the API should be --lock-write as a bool (and require it be used in conjunction with the other flag specifying the lock file)? When would you check and write with two distinct files?

i.e.

--lock=lock.json  # currently --lock-check
--lock=lock.json --lock-write # currently --lock-write
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Oct 29, 2019

@hayd Maybe... What is the behavior if deno is provided only --lock=lock.json but the lock file does not yet exist?

ry added 2 commits Oct 29, 2019
@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 29, 2019

@ry error! (with a message)

ry added 5 commits Oct 29, 2019
fmt
@ry ry requested a review from piscisaureus Oct 29, 2019
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Oct 29, 2019

@hayd, I implemented your flag suggestion.

Should --lock-write be combined with --reload?

Should file:// modules be written into the lock file and checked? Or can we trust local code?

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 29, 2019

Should --lock-write be combined with --reload?

I'm not sure, I think maybe not... we can decide later.

Reason maybe not: someone else updates the lockfile in git, you update, you have an old url cached e.g. https://deno.land/std/foo.ts so you need to --reload BUT you want to ensure the lockfile is unmodified (that you are running the same configuration as your colleague)... if not you want it to error.

Should file:// modules be written into the lock file and checked? Or can we trust local code?

I'm not sure. Need to think about that. I suspect you should trust it as otherwise the lock file will be incredibly noisy/a pain, and it will cause issues in git etc.

ry added 2 commits Oct 29, 2019
Copy link
Contributor

bartlomieju left a comment

Awesome, I'm wondering about ergonomics though.

It'd be nice if you could just specify --lock file without explicit path (otherwise everyone will have different lock filename, might be a good idea to give people a cue towards common name).

$ deno --lock --lock-write ./my_script.ts
$ cat Deno.lock
{
  "http://127.0.0.1:4545/cli/tests/subdir/print_hello.ts": "5c93c66125878389f47f4abcac003f4be1276c5223612c26302460d71841e287",
  "http://127.0.0.1:4545/cli/tests/003_relative_import.ts": "bad"
}

That way --lock-write wouldn't require --lock flag.

Anyway LGTM, just some food for thought.

cli/lib.rs Show resolved Hide resolved
cli/state.rs Show resolved Hide resolved
@nayeemrmn

This comment has been minimized.

Copy link
Contributor

nayeemrmn commented Oct 29, 2019

It'd be nice if you could just specify --lock file without explicit path (otherwise everyone will have different lock filename, might be a good idea to give people a cue towards common name).

A lock file will correspond to a module, right? Building on that, it would be nice if the lock file was written next to the specified main module as such: deno fetch --lock-write src/main.ts creates src/main.ts.lock.
(This opens the door to making --lock-check implicit, if that hasn't been ruled out 😊)

std/manual.md Show resolved Hide resolved
@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Oct 29, 2019

Awesome, I'm wondering about ergonomics though. It'd be nice if you could just specify --lock file without explicit path (otherwise everyone will have different lock filename, might be a good idea to give people a cue towards common name).

I think most of the time when developing one does not care about the lock file. It's only during deployment and testing that this is important - and usually in that case the invocation is scripted. I think this explicit file name specification might not be such a problem. Also I don't like the idea of blessing a special file name like "Deno.lock". There are not any special filenames in Deno, I want to strive to not introduce any if possible. Then again - I could be wrong - I think we just have to play with this a bit and see what the ergonomics are like in real-life.

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 29, 2019

I think this should work with fetch as well.

@ry

This comment has been minimized.

Copy link
Collaborator Author

ry commented Oct 29, 2019

@hayd Do you mean --lock=https://example.com/lock.json ? That's a potential feature, but I don't want to do it in this PR.

@bartlomieju

This comment has been minimized.

Copy link
Contributor

bartlomieju commented Oct 29, 2019

@hayd Do you mean --lock=https://example.com/lock.json ? That's a potential feature, but I don't want to do it in this PR.

I think @hayd meant it should work with deno fetch, which I think it already does (lock is a global flag)

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 29, 2019

I think another option is deps.lock.json so you'd do

deno fetch --lock=deps.lock.json deps.ts
deno run --lock=deps.lock.json app.ts

That way forcing all external imports be included in deps.ts.

I don't think we need to pick a winner, I also suspect higher level tooling will control this, along with import maps for a nicer experience.

There is an interesting question re lockfiles as trees, i.e. if I import x/foo what files did it expect as its lockfile. Again, I think you could do lockfile "verification" as higher level, esp. since you can already get the dep tree separately.

Edit: Perhaps this mentioned in a different thread (for some reason I was thinking about it), the idea was if you import from a registry e.g. deno.land/std/foo.ts it could tell you where the lockfile is for that dependency/module, and you could verify that your lockfile was a subset. I'm not completely sure how that would look, but it'd be higher level, on top of lockfile+info.

@hayd

This comment has been minimized.

Copy link
Contributor

hayd commented Oct 29, 2019

was just checking. I think this is looking great.

.takes_value(true)
.global(true),
).arg(
Arg::with_name("lock-write")

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Nov 2, 2019

Collaborator

I think it may be more user friendly to do deno --lock-write lockfile.json rather than deno --lock lockfile.json --lock-write.
I'd be okay with doing this in in a follow-up PR tho.

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

piscisaureus commented Nov 2, 2019

Sorry for the delay; I needed some time to consider whether this implementation would interfere with a potential future addition of signature support, but I've come to the conclusion that there isn't a problem.

Copy link
Collaborator

piscisaureus left a comment

LGTM with one comment.
Note that this PR needs to be rebased because CI configuration has changed after it was submitted.

@ry ry merged commit 86b3ac5 into denoland:master Nov 3, 2019
10 checks passed
10 checks passed
test macOS-latest
Details
test_std macOS-latest
Details
test windows-2019
Details
test_std windows-2019
Details
test ubuntu-16.04
Details
test_debug ubuntu-16.04
Details
test_std ubuntu-16.04
Details
bench ubuntu-16.04
Details
lint ubuntu-16.04
Details
license/cla Contributor License Agreement is signed.
Details
@ry ry deleted the ry:lock_file branch Nov 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.