This repository has been archived by the owner. It is now read-only.

Move execution of top level lifecycle scripts to install instead of postinstall #13259

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@palmerj3
Contributor

palmerj3 commented Jul 3, 2016

This appears to fix #10379

Could use some further testing.

palmerj3 added some commits Jul 3, 2016

Move execution of top level lifecycle scripts to install instead of p…
…ostinstall

Change-Id: I17e28002d53937d07b90b210ffbcd8b5b1ca585a
Moved runTopLevelLifecycles above executeActions
Change-Id: I84d71d01809f20990f8ba08613b3b09df24aedc3
@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 3, 2016

The AppVeyor failure seems to be an issue with the CI server not having access to a git repo. Can you have a look and re-trigger? ping @iarna

@DenianArthurShades

This comment has been minimized.

DenianArthurShades commented Jul 4, 2016

I'll have to check tonight to be sure, but just in case I forget: Are you sure the top level lifecycle scripts don't include the postinstall script? Because just like preinstall should happen before install, postinstall should happen afterwards...

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 4, 2016

Yes it does include the postinstall script but it also runs build before hand..

What I can do is refactor topLevelScripts into two methods pre/post and run them before/after executeActions and that should be valid.

Split runTopLevelLifecycles into pre and post methods and execute in …
…the proper order

Change-Id: I9f7a709c1aada32e02c87514019480e4ddd2e2fb
@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 4, 2016

@DenianArthurShades I did a slight refactor, as mentioned, and now runTopLevelLifecycles is split into pre and post methods and executed in the proper order. Please let me know if you're able to test this. Thanks!

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 6, 2016

@seldo @iarna can you have a look?

Added test to verify preinstall, install, and postinstall lifecycle s…
…cript execution order and state

Change-Id: I995435e1aad586ce2a72291bf633f1256801686d
@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 7, 2016

Added a simple test to verify that lifecycle scripts are executed in the proper order and that node_modules does not exist (on a clean install of an app) on preinstall task.

Removed semicolon from lifecycle-order tap
Change-Id: Icc1d30545f259be281bf2eec5644cdb48a4aed3b

@KenanY KenanY added the lifecycle label Jul 7, 2016

@iarna

This comment has been minimized.

Member

iarna commented Jul 7, 2016

Thank you for putting this together. Unfortunately, this is, I believe, insufficient to the practical needs of a preinstall lifecycle. Specifically, my understanding is that some of the things folks want to be able to do include installing modules or otherwise modifying the tree before the installer starts. To do that, this needs to execute before the currentTree is loaded off disk, which is substantially earlier in the process.

@iarna iarna added the in-progress label Jul 7, 2016

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 7, 2016

@iarna ok what is required? Are we going off of what people want or some agreed upon spec?

I'll do whatever is necessary to get this bug fixed.

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 7, 2016

@iarna also with this PR I am still able to install modules in the preinstall step...

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Jul 13, 2016

@iarna let me know your thoughts on this. I can take this further if necessary but thus far I think my PR works as expected.

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Aug 4, 2016

I would really appreciate a more thorough explanation here as to why this isn't adequate for merging... I'm willing to put in the time to make adjustments but the one comment I've received thus far is fairly ambiguous and is questionable whether it's even necessary.

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Aug 5, 2016

Last comment: I will just put this here that your own documentation says preinstall should only be used for compilation not for installing dependencies.. so I would really rather us not introduce feature creep into this and fix a bug that's existed the entire major version 3.

screen shot 2016-08-05 at 3 32 39 pm

@iarna

This comment has been minimized.

Member

iarna commented Aug 8, 2016

@palmerj3 Thank you for your patience. I don't disagree with you per se, but it's not exactly installs people want to do here. I believe the main desire is to be able to setup symlinks in the node_modules that npm will preserve. The reasons I outlined previously are my summation of previous issues around this. Fixing this as you have would solve some but not all of the concerns folks have raised about the current behavior.

That being said, as I've thought this through more I think I'm inclined to say "no" to the "can read the node_modules AFTER preinstall time" because while that works fine for applications, it would be a disaster to implement for things being installed as dependencies, and I'm not comfortable having those two modes work differently.

To solve that need, I would prefer to have some application lifecycles that don't apply when you're working with something as a dependency. But obviously that's out of scope for this.

So on reflection I think I am ok with this as it is. Uh, sorry for taking so long to come around on this. =D I have a few minor tweaks I'd like to see in this PR, but they're small enough I'm happy to make them at merge time.

@@ -259,6 +259,10 @@ Installer.prototype.run = function (cb) {
[this, this.debugActions, 'decomposeActions', 'todo'])
if (!this.dryrun) {
installSteps.push(
[this.newTracker(log, 'runPreinstallTopLevelLifecycles', 2)],

This comment has been minimized.

@iarna

iarna Aug 8, 2016

Member

Specifically: We don't need a new type of tracker, we can use runTopLevelLifecycles across both pre and postinstall lifecycles. Multiple completion trackers can be active simultaneously.

var steps = []
var trackPreinstallLifecycle = this.progress.runPreinstallTopLevelLifecycles
if (!this.topLevelLifecycles) {
trackPreinstallLifecycle.finish()

This comment has been minimized.

@iarna

iarna Aug 8, 2016

Member

This is already being finished on line 264– there's no need for this.

@palmerj3

This comment has been minimized.

Contributor

palmerj3 commented Aug 8, 2016

@iarna ok cool! Let me know if you need me to make those changes.

@iarna iarna added target-latest and removed in-progress labels Aug 8, 2016

@iarna iarna added this to the next milestone Aug 11, 2016

iarna added a commit that referenced this pull request Aug 11, 2016

@iarna

This comment has been minimized.

Member

iarna commented Aug 11, 2016

This has been merged and will be in the next release (scheduled for later today).

@iarna iarna closed this Aug 11, 2016

iarna added a commit that referenced this pull request Aug 11, 2016

@zkat zkat referenced this pull request Sep 22, 2016

Closed

deps: upgrade npm to 3.10.8 #8706

4 of 4 tasks complete

@palmerj3 palmerj3 deleted the palmerj3:fixPreinstall branch Dec 17, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.