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

Fix missing deduplicate command when using build:core #7621

Merged
merged 1 commit into from
Dec 23, 2019

Conversation

lresende
Copy link
Member

Code changes

Fix typo on deduplicate command. deduplicate should be yarn-deduplicate

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@lresende lresende added this to the 2.0 milestone Dec 12, 2019
@telamonian
Copy link
Member

Is this what we actually want? What role does build:core currently play in the build system? And what is the general policy about when to dedupe?

I for one am fairly confused. In my own workflow, I never run build:core, and I only dedupe when there's an obvious problem. Aside from performance issues, is there a good reason not to always dedupe as part of integrity?

@lresende
Copy link
Member Author

lresende commented Dec 13, 2019

Good question @telamonian, note that without this fix build:core was giving an error deduplicate not found and continuing. This fix mainly address that issue. As for your broader question, I guess I let others chime in.

@telamonian
Copy link
Member

telamonian commented Dec 13, 2019

@saulshanabrook @blink1073 What do you guys think? I'm leaning towards the idea that dedupe should be part of integrity, since it affects yarn.lock

Also, is build:core still actually used in any of our build workflows?

@saulshanabrook
Copy link
Member

What do you guys think? I'm leaning towards the idea that dedupe should be part of integrity, since it affects yarn.lock

I don't really know what the dedupe command does. I also don't know when to run build:core

@telamonian
Copy link
Member

I also don't know when to run build:core

I'm pretty sure that build:core is completely useless in the current build system, though anyone please correct me if that's wrong.

jlpm build:core builds the jlab bundle in the staging dir within the repo itself. So far as I can tell, said bundle will never get loaded by any variation of the jupyter lab --whatever cmd, nor does it get used by anything else in the build.

@jasongrout
Copy link
Contributor

jlpm build:core builds the jlab bundle in the staging dir within the repo itself. So far as I can tell, said bundle will never get loaded by any variation of the jupyter lab --whatever cmd, nor does it get used by anything else in the build.

I think that would get used if you did pip install -e and jupyter lab --core-mode to run from the staging directory.

@vidartf
Copy link
Member

vidartf commented Dec 15, 2019

I think this is intended to run a command similar to the one in the root package. I think we will need some careful digging in the git history to figure out why it ended up like this.

@blink1073
Copy link
Member

@vidartf is right that it is calling the deduplicate script in the root package.
Staging has an independent yarn.lock that is created during the publish process, which is why we explicitly need to deduplicate it here.
The build:core command is run as part of the publish process. The assets in staging are also what ends up in share/jupyter/lab/.
I think we can close this with no action.

@blink1073 blink1073 closed this Dec 17, 2019
@lresende
Copy link
Member Author

So, if I follow the steps on contributing.md, and perform:

git clone https://github.com/<your-github-username>/jupyterlab.git
cd jupyterlab
pip install -e .
jlpm install
jlpm run build  # Build the dev mode assets (optional)
jlpm run build:core  # Build the core mode assets (optional)
jupyter lab build  # Build the app dir assets (optional)

I would get:

...
[3/5] 🚚  Fetching packages...
[4/5] 🔗  Linking dependencies...
[5/5] 🔨  Building fresh packages...
success Saved lockfile.
**error Command "deduplicate" not found.**
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
[1/5] 🔍  Validating package.json...
[2/5] 🔍  Resolving packages...
success Already up-to-date.

Is this error the expected result? Are there possible side effects related to not performing the deduplication?

@vidartf
Copy link
Member

vidartf commented Dec 18, 2019

@blink1073 I think what @lresende is pointing out is that the package.json file in staging does not have the dedupe script defined.

@vidartf vidartf reopened this Dec 18, 2019
@blink1073
Copy link
Member

Yeah, that's fair. Copying the script from the root package.json to dev_mode/package.json makes sense to me.

@lresende
Copy link
Member Author

Ok, I going to update the pr with the suggestion above a little later today.

@lresende lresende changed the title Fix deduplicate command typo Fix missing deduplicate command when using build:core Dec 19, 2019
@blink1073
Copy link
Member

Thanks!

@blink1073 blink1073 merged commit f4995f9 into jupyterlab:master Dec 23, 2019
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Jan 24, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jan 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants