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

Post commands not longer follow execution order #4855

Closed
thaney071 opened this issue Apr 8, 2021 · 14 comments
Closed

Post commands not longer follow execution order #4855

thaney071 opened this issue Apr 8, 2021 · 14 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug containers Issue in vscode-remote containers verified Verification succeeded
Milestone

Comments

@thaney071
Copy link

  • VSCode Version: 1.55.x
  • Local OS Version: Ubuntu 20.04
  • Remote OS Version: Ubuntu focal
  • Remote Extension/Connection Type: Docker

Since #4446 was completed, the execution order of the post command has changes. This causes problems for start up that is order dependent. In our case we need to modify the permissions for the go directory so that the user has access to the folder. Previously the post create command would run before any of the vscode extensions started running. Now since the create command runs in the background, we get errors from our linters because the directory for where modules is stored is not accessible when the gopls server starts to run.

In general, that changed the start up behavior of the post commands since they are now running at the same time we cannot use them to prepare the container in a particular way before other things are allowed to start.

Does this issue occur when you try this locally?: N/A
Does this issue occur when you try this locally and all extensions are disabled?: N/A

@github-actions github-actions bot added the containers Issue in vscode-remote containers label Apr 8, 2021
@felipecrs
Copy link

I'm suffering of the same issue: the Python extension tries to load python from my .venv folder (as I have specified it in the .vscode/settings.json), but it's not yet created and therefore it fails.

Because of that, I have to wait until the postCreateCommand finishes and then reload the window. Very annoying.

An approach different than restoring the old behavior would be to add a new option which would respect this workflow, something like:

runPostCreateCommandInBackground: false

Of course the name could be improved.

@chrmarti
Copy link
Contributor

Can you move what you are doing in "postCreateCommand" to the Dockerfile? (Introduce a Dockerfile and refer to it with the "build" property in the devcontainer.json if you don't have one already.)

@chrmarti chrmarti self-assigned this Apr 14, 2021
@chrmarti chrmarti added the info-needed Issue requires more information from poster label Apr 14, 2021
@felipecrs
Copy link

felipecrs commented Apr 15, 2021

How would that work? I mean, isn't VSCode mounting the workspace as volume to the container? Copying workspace, running some steps on it, then mounting it on top of the files copied (and possibly changed), what would that cause?

Also, COPY uses .dockerignore, which are supposed to prune files not needed in production. Here we are in dev mode, we don't want to prune such files.

Maybe I'm missing something.

@chrmarti
Copy link
Contributor

I think this really depends on your particular case.

So the request is for a property in devcontainer.json to disable parallelization of the post-create stage. There are also dotfiles, postStartCommand and postAttachCommand being run as part of that stage and we could add a flag for each, but I would prefer a single flag.

E.g.: "awaitPostCreateStage": boolean with the default being false. Maybe there is a better name.

/cc @Chuxel @egamma

@Chuxel
Copy link
Member

Chuxel commented Apr 16, 2021

@chrmarti I think there's kind of two things here. One is whether to wait for dotfiles, the other is whether serialize lifecycle events. Dotfiles cross repos, so I think that needs to be separate. Dotfiles also seems like a user setting, while the other is a devcontainer.json setting. Obviously the "settings" object could also be used to force dotfiles behaviors as a user setting.

@felipecrs
Copy link

To avoid adding a new configuration object, something like the following could also be done

{
  "postCreateCommand": "npm install", // a string, as before
},
// or
{
  "postCreateCommand": [ "npm install", "beforeWindowLoad" ] // a tuple of two strings, the command line and the "mode"
}
// or
{
  "postCreateCommand": {
     "commandLine": "npm install",
     "mode": "beforeWindowLoad" 
  } // an object
}

And the extension could handle different inputs.

@thaney071
Copy link
Author

Can you move what you are doing in "postCreateCommand" to the Dockerfile? (Introduce a Dockerfile and refer to it with the "build" property in the devcontainer.json if you don't have one already.)

That doesn't work because vscode is what updates the UID and GID information inside the container after the dockerfile is built.
So running chown vscode:vscode /go does not set the expected UID and GID for that directory.

@chrmarti
Copy link
Contributor

chrmarti commented May 4, 2021

@thaney071 Not sure if that's an option, but we chown -R the user's home folder to have the updated UID:GID ownership. If you place the go folder in there and a symlink at /go, that might work around this.

@Chuxel I was thinking for simplicity, we could have a single flag to wait for all 'post create' operations (postCreateCommand, dotfiles, postStartCommand, postAttachCommand) to finish, like we used to do. dotfiles is currently not separate because we install them before postStart/AttachCommand, so these two run after dotfiles are installed not only on second and later, but also on first run. This is to ensure consistent results.

@chrmarti chrmarti added bug Issue identified by VS Code Team member as probable bug under-discussion Issue is under discussion for relevance, priority, approach and removed info-needed Issue requires more information from poster labels May 4, 2021
@chrmarti chrmarti added this to the Backlog milestone May 4, 2021
@Chuxel
Copy link
Member

Chuxel commented May 4, 2021

@chrmarti Got it. Or as discussed we could introduce a step that does block before VS Code server spins up along with this that does not for scenarios where blocking is needed.

@Chuxel
Copy link
Member

Chuxel commented Jun 8, 2021

@chrmarti I think we can consider this implemented with the upcoming waitFor property that's already available in insiders and soon to be released, correct?

@chrmarti
Copy link
Contributor

chrmarti commented Jun 8, 2021

Indeed, you could set "waitFor": "postCreateCommand" or you could move the command that needs to be waited for to the new "onCreateCommand" property.

@chrmarti chrmarti closed this as completed Jun 8, 2021
@chrmarti chrmarti removed the under-discussion Issue is under discussion for relevance, priority, approach label Jun 8, 2021
@chrmarti chrmarti modified the milestones: Backlog, May 2021 Jun 8, 2021
@chrmarti chrmarti added the author-verification-requested Issues potentially verifiable by issue author label Jun 8, 2021
@chrmarti
Copy link
Contributor

chrmarti commented Jun 8, 2021

@thaney071 @felipecrs Could you check if that works for you? (Currently requires VS Code Insiders and will be available with VS Code 1.57.)

@felipecrs
Copy link

Awesome, it works. Thank you!

@rzhao271 rzhao271 added verified Verification succeeded and removed author-verification-requested Issues potentially verifiable by issue author labels Jun 8, 2021
@rzhao271
Copy link

rzhao271 commented Jun 8, 2021

Marked as verified based on comment above.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug containers Issue in vscode-remote containers verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

5 participants