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
Merged

feat: lockfiles #3231

merged 14 commits into from
Nov 3, 2019

Conversation

ry
Copy link
Member

@ry 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
@hayd
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
Copy link
Member 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?

@hayd
Copy link
Contributor

hayd commented Oct 29, 2019

@ry error! (with a message)

@ry ry requested a review from piscisaureus October 29, 2019 06:19
@ry
Copy link
Member Author

ry commented Oct 29, 2019

@hayd, I implemented your flag suggestion.

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

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

@hayd
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:https:// 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.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:https://127.0.0.1:4545/cli/tests/subdir/print_hello.ts": "5c93c66125878389f47f4abcac003f4be1276c5223612c26302460d71841e287",
  "http:https://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
Copy link
Collaborator

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 😊)

@ry
Copy link
Member 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
Copy link
Contributor

hayd commented Oct 29, 2019

I think this should work with fetch as well.

@ry
Copy link
Member 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
Copy link
Member

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
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
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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

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
Member

@piscisaureus piscisaureus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@ry ry deleted the lock_file branch November 3, 2019 17:49
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 21, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 24, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Jan 31, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
caspervonb pushed a commit to caspervonb/deno_std that referenced this pull request Feb 1, 2021
Use --lock-write=lock.json or --lock-check=lock.json on the command
line.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Subresource integrity for modules
5 participants