Migrated Builder Folder to jupyter-builder #29

Darshan808 · 2024-10-22T08:23:47Z

Fixes #16

In this PR, I have

Migrated the builder folder from the JupyterLab repository to the jupyter-builder package.
Modified the script to use jupyter-builder from the extension’s node_modules if it is presen.

Looking forward to your suggestions for moving forward with jupyter-builder development.

Darshan808 · 2024-10-22T09:15:30Z

@fcollonval
I had a couple of questions for clarification:

Previously, we used to extract the version of @jupyterlab/builder from both the extension’s package.json and the staging package.json (core) to check for compatibility. Now, will we be extracting the version of @jupyterlab-builder/builder from both and checking for compatibility in the same way?
Since we plan to replace jupyterlab in the build dependencies of the extension template with jupyter-builder, does this mean that in a completely new environment, we will be able to build extensions without needing to install the jupyterlab package? If the answer is yes, I have some more doubts, like how can we check core's compatibility without jupyterlab?

fcollonval

Thanks for opening this PR @Darshan808

My first request will be to reorganize the code to not use a subfolder builder but directly the existing src folder.

This implies to merge the tsconfig.json and package.json into the root one.

Then you will need to uncomment the following block:

jupyter-builder/jupyter_builder/federated_extensions.py

Lines 298 to 312 in f7df010

    
           # overlap = _test_overlap( 
        
           #     dep_version1, dep_version2, drop_prerelease1=True, drop_prerelease2=True 
        
           # ) 
        
           # if not overlap: 
        
           #     with open( 
        
           #         osp.join(target, "node_modules", "@jupyterlab", "builder", "package.json") 
        
           #     ) as fid: 
        
           #         dep_version2 = json.load(fid).get("version") 
        
           #     overlap = _test_overlap( 
        
           #         dep_version1, dep_version2, drop_prerelease1=True, drop_prerelease2=True 
        
           #     ) 
        
           # if not overlap: 
        
           #     msg = f"Extensions require a devDependency on @jupyterlab/builder@{dep_version1}, you have a dependency on {dep_version2}"  # noqa: E501 
        
           #     raise ValueError(msg)

and do any changes to make it works; aka checking that the CI is passing.

Don't hesitate to ping me directly on Gitter for faster replies 😉
--> https://matrix.to/#/@fcollonval-5b7f8ee9d73408ce4fa5a522:gitter.im

fcollonval · 2024-10-22T09:42:17Z

Regarding your questions

Previously, we used to extract the version of @jupyterlab/builder from both the extension’s package.json and the staging package.json (core) to check for compatibility. Now, will we be extracting the version of @jupyterlab-builder/builder from both and checking for compatibility in the same way?

Indeed, we gonna need to check the version of @jupyterlab-builder is compatible between this package and JupyterLab core and between this package and the extension.

Since we plan to replace jupyterlab in the build dependencies of the extension template with jupyter-builder, does this mean that in a completely new environment, we will be able to build extensions without needing to install the jupyterlab package? If the answer is yes, I have some more doubts, like how can we check core's compatibility without jupyterlab?

No JupyterLab will still be needed. The end goal of this package is to ease the builder maintenance code and later to support other applications. But the dependency on the application (aka for now JupyterLab) will still be needed in the extension.

To be more concrete, currently an extension for JupyterLab specifies in package.json configuration like:

"dependencies" {
  "@jupyterlab/application": "^4.0.0"
},
"devDependencies": {
  "@jupyterlab/builder": "^4.0.0"
},
"jupyterlab": {
  "extension": true
}

That will be kept - although an additional package will likely be needed:

"dependencies" {
  "@jupyterlab/application": "^4.0.0"
},
"devDependencies": {
  "@jupyterlab/builder": "^4.0.0",
+  "@jupyterlab/core-meta": "^4.0.0"
},
"jupyterlab": {
  "extension": true
}

However, in pyproject.toml, it will be possible to swap jupyterlab by jupyter-builder reducing the build dependencies stack:

  [build-system]
- requires = ["hatchling>=1.5.0", "jupyterlab>=4.0.0,<5", "hatch-nodejs-version>=0.3.2"]
+ requires = ["hatchling>=1.5.0", "jupyter-builder>=4.0.0,<5", "hatch-nodejs-version>=0.3.2"]
  build-backend = "hatchling.build"

Darshan808 · 2024-10-22T14:16:14Z

@fcollonval
Thanks for the clarification!
I've made the requested changes. To handle version compatibility, I had to bring additional functions from the core's semver.py into jupyterlab_semver.py. Could you please review it now?

Darshan808 · 2024-10-23T07:30:01Z

@fcollonval
Requesting a re-trigger—this should hopefully be the last one for this PR. Thanks for your understanding!

fcollonval

Thanks @Darshan808 I have two major requests.

jupyter_builder/jupyterlab_semver.py

.eslintrc.js

fcollonval

Thanks @Darshan808

I have two alternative suggestions to avoid skipping linter rule for require(). And then we are in a good shape to merge this.

A good test to ad would be the matching of the @jupyterlab/builder, would you be up to the task? We could do that in a follow-up PR.

src/extensionConfig.ts

Darshan808 · 2024-10-29T16:46:09Z

A good test to ad would be the matching of the @jupyterlab/builder, would you be up to the task? We could do that in a follow-up PR.

@fcollonval
Yes, I’m up to the task! Could you please clarify what you mean by ‘matching of the @jupyterlab/builder? I just want to make sure I understand it fully. Thank you!

fcollonval · 2024-10-30T08:05:02Z

Yes, I’m up to the task! Could you please clarify what you mean by matching of the @jupyterlab/builder?

The version matching you restored in this PR. Do you see how to do that?

Darshan808 · 2024-10-30T10:27:22Z

The version matching you restored in this PR. Do you see how to do that?

The version matching I restored in this PR involved checking the compatibility between the @jupyterlab/builder version used in the core and the one installed in the extension's node modules.
Regarding the test you're mentioning, what specific version are we matching the @jupyterlab/builder against? Could you provide some clarification on this?

fcollonval · 2024-10-31T13:15:18Z

Regarding the test you're mentioning, what specific version are we matching the @jupyterlab/builder against? Could you provide some clarification on this?

The test done in the code is a version range matching between the @jupyterlab/builder package version shipped within this library at

jupyter-builder/package.json

Line 3 in f7df010

"version": "0.0.1",

and the range provided by the extension at https://github.com/jupyterlab/extension-template/blob/5696924f5b7adf07cf07ea9d3510c66914fbb770/template/package.json.jinja#L68.

So the test should have the following steps:

Set up a extension from the template
Modify the range of @jupyterlab/builder to be incompatible with the current version
Check that the expected error is raised

Existing test are located at https://github.com/jupyterlab/jupyter-builder/blob/main/tests/test_tpl.py
That should help you with the first step. Let me know if you need help for the others steps.

Darshan808 · 2024-10-31T18:35:44Z

That should help you with the first step. Let me know if you need help for the others steps.

Understood! With the major festive season in Nepal, it's a bit challenging to focus fully at the moment, but I’ll make sure to complete it in the next few days.

Darshan808 · 2024-11-04T11:20:58Z

Added a test to validate version compatibility.

The test modifies the version range for @jupyterlab/builder in the extension then attempts to build the extension.
During this process, we verify that the expected error related to version mismatches is raised.

Note: I have currently set the extension's version range to exactly 4.0.0. This can be adjusted for optimal compatibility.

Please review and suggest any changes or modifications.

Darshan808 · 2024-11-04T12:28:35Z

@fcollonval
I'm getting the expected warning locally for the version matching test and the test passes locally. Not sure why it’s failing on CI, but I’m looking into it. Any insights would be helpful!

Darshan808 · 2024-11-04T13:21:37Z

@fcollonval
Tests are all passing now! Could you review the code and share any feedback?

fcollonval

Thanks a lot @Darshan808 for the test - and congrats to make it work right away.

I have a minor suggestion. But otherwise we are good to go.

fcollonval · 2024-11-05T07:57:09Z

tests/test_tpl.py

+    with package_json_path.open("r+", encoding="utf-8") as f:
+        package_data = json.load(f)
+        package_data["devDependencies"]["@jupyterlab/builder"] = "4.0.0"
+        f.seek(0)
+        json.dump(package_data, f, indent=2)
+        f.truncate()


It is better to not hold on a file.

You could simplify and make easier to read that snippet using:

Suggested change

with package_json_path.open("r+", encoding="utf-8") as f:

package_data = json.load(f)

package_data["devDependencies"]["@jupyterlab/builder"] = "4.0.0"

f.seek(0)

json.dump(package_data, f, indent=2)

f.truncate()

package_data = json.loads(package_json_path.read_text())

package_data["devDependencies"]["@jupyterlab/builder"] = "4.0.0"

package_json_path.write_text(json.dumps(package_data, indent=2))

Thanks for the suggestion! I made the changes, and it looks much better now.

fcollonval

Thanks a lot for the patience @Darshan808

Congrats on the first PR for this repo.

Darshan808 · 2024-11-05T16:47:52Z

@fcollonval
Just wanted to say a huge thank you for all the guidance and support throughout this PR. Mentors like you make such a big difference, especially for us newer open-source contributors—it keeps us motivated to keep pushing forward.

Since I’m now part of the JupyterLab triage team, I’m even more excited to keep contributing! What's next for us here, or do we have any plans on when core might start using this package?

jtpio · 2024-11-05T16:59:23Z

Thanks @Darshan808 for the contribution and @fcollonval for the guidance!

What's next for us here, or do we have any plans on when core might start using this package?

Now that JupyterLab 4.3.0, there is active work on the next 4.4.0 version. I haven't caught up with the latest activity on this jupyter-builder project, but maybe it would be a good candidate for the 4.4.0 release?
Doing the switch sooner than later would also help avoid having to maintain the same code in two different places, and avoid potential divergences.

Darshan808 · 2024-11-05T18:41:16Z

Sounds like a great idea, and I'm definitely on board! I believe I can contribute effectively with a bit of guidance from @fcollonval. Here are some steps i think needs to be done:

Add jupyter-builder as a dependency in the core
Update the build scripts to utilize jupyter-builder
Remove the existing builder from the core

I would appreciate any feedback on this and guidance on the steps I should take to move forward!

Darshan808 · 2024-11-07T16:20:30Z

@fcollonval
Should I open an issue here or in the JupyterLab repo to discuss transitioning the core to use this builder? When are we planning to start this process? If you could provide a structure or roadmap, I’d be happy to start working on it.

Darshan808 and others added 4 commits October 22, 2024 13:23

builder-migration

7571b71

script modification

03a4cdd

script modification

7764389

Automatic application of license header

eb70e9f

fcollonval added the enhancement New feature or request label Oct 22, 2024

fcollonval suggested changes Oct 22, 2024

View reviewed changes

Darshan808 and others added 5 commits October 22, 2024 19:29

organized-build-files

6603df0

remove-automatic-application-of-license-header-on-yarn.js

101185d

remove-yarn-license-header

2f37b6d

Merge branch 'jupyterlab:main' into jupyter-builder-enhancement

42d2fd0

remove-unused-dependencies

49f0e47

Darshan808 added 2 commits October 22, 2024 21:06

add-eslint-files

5f10e1b

ruff-error-fixed

bd40b1b

Darshan808 requested a review from fcollonval October 22, 2024 16:25

correct-spellings

d4b7224

fcollonval suggested changes Oct 24, 2024

View reviewed changes

jupyter_builder/jupyterlab_semver.py Outdated Show resolved Hide resolved

.eslintrc.js Outdated Show resolved Hide resolved

fix-lints

68cae21

fcollonval reviewed Oct 29, 2024

View reviewed changes

src/extensionConfig.ts Outdated Show resolved Hide resolved

src/extensionConfig.ts Outdated Show resolved Hide resolved

removed-lint-check-skips

f382e22

add test for version match

7f9d608

test version modification

a7fc6e6

test modification

2e57483

fcollonval suggested changes Nov 5, 2024

View reviewed changes

test modification

480e234

fcollonval approved these changes Nov 5, 2024

View reviewed changes

fcollonval merged commit c5277f4 into jupyterlab:main Nov 5, 2024
20 checks passed

	# overlap = _test_overlap(
	# dep_version1, dep_version2, drop_prerelease1=True, drop_prerelease2=True
	# )
	# if not overlap:
	# with open(
	# osp.join(target, "node_modules", "@jupyterlab", "builder", "package.json")
	# ) as fid:
	# dep_version2 = json.load(fid).get("version")
	# overlap = _test_overlap(
	# dep_version1, dep_version2, drop_prerelease1=True, drop_prerelease2=True
	# )

	# if not overlap:
	# msg = f"Extensions require a devDependency on @jupyterlab/builder@{dep_version1}, you have a dependency on {dep_version2}" # noqa: E501
	# raise ValueError(msg)

Uh oh!

Migrated Builder Folder to jupyter-builder #29

Migrated Builder Folder to jupyter-builder #29

Uh oh!

Conversation

Darshan808 commented Oct 22, 2024 • edited by fcollonval Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Darshan808 commented Oct 22, 2024

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

fcollonval commented Oct 22, 2024

Uh oh!

Darshan808 commented Oct 22, 2024

Uh oh!

Darshan808 commented Oct 23, 2024

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Darshan808 commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fcollonval commented Oct 30, 2024

Uh oh!

Darshan808 commented Oct 30, 2024

Uh oh!

fcollonval commented Oct 31, 2024

Uh oh!

Darshan808 commented Oct 31, 2024

Uh oh!

Darshan808 commented Nov 4, 2024

Added a test to validate version compatibility.

Uh oh!

Darshan808 commented Nov 4, 2024

Uh oh!

Darshan808 commented Nov 4, 2024

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

fcollonval Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

Darshan808 Nov 5, 2024

Choose a reason for hiding this comment

Uh oh!

fcollonval left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Darshan808 commented Nov 5, 2024

Uh oh!

jtpio commented Nov 5, 2024

Uh oh!

Darshan808 commented Nov 5, 2024

Uh oh!

Darshan808 commented Nov 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Darshan808 commented Oct 22, 2024 •

edited by fcollonval

Loading

Darshan808 commented Oct 29, 2024 •

edited

Loading