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

Platform-specific vsix #89

Merged
merged 41 commits into from
Oct 18, 2023
Merged

Platform-specific vsix #89

merged 41 commits into from
Oct 18, 2023

Conversation

paulacamargo25
Copy link
Contributor

@paulacamargo25 paulacamargo25 commented Sep 21, 2023

@paulacamargo25 paulacamargo25 added this to the September 2023 milestone Sep 21, 2023
@paulacamargo25 paulacamargo25 added the debt Covers everything internal: CI, testing, refactoring of the codebase, etc. label Sep 21, 2023
@paulacamargo25 paulacamargo25 self-assigned this Sep 21, 2023
@paulacamargo25 paulacamargo25 marked this pull request as ready for review September 21, 2023 20:23
@@ -28,6 +28,34 @@ extends:
template: azure-pipelines/extension/pre-release.yml@templates
parameters:
l10nSourcePaths: ./src
buildPlatforms:
- name: Linux
vsceTarget: ''
Copy link

Choose a reason for hiding this comment

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

Is this line needed? There is no vsceTarget?

Copy link
Member

Choose a reason for hiding this comment

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

This will create a platform independent VSIX. @paulacamargo25, is that the intention here? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, according to what I understood, I always need one that does not depend on any platform, correct?

Copy link
Member

Choose a reason for hiding this comment

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

@isidorn, @sandy081 to confirm that.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, according to what I understood, I always need one that does not depend on any platform, correct?

What does this platform independent VSIX contain? It is not necessary if you are building VSIXs for all platforms.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, it contains the debugpy wheel which is not specific to any platform.

Copy link
Member

Choose a reason for hiding this comment

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

If you are building VSIX for all platforms then you do not need a general VSIX.

@isidorn isidorn requested a review from lszomoru September 22, 2023 09:09
isidorn
isidorn previously approved these changes Sep 22, 2023
Copy link

@isidorn isidorn left a comment

Choose a reason for hiding this comment

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

Overall the PR looks good to me. I am also assigning to @lszomoru so he gives it a glance since he owns the extension publish pipelines.

sandy081
sandy081 previously approved these changes Sep 25, 2023
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

There's a critical miss of exiting on failure of hash validation.

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Some clean-up in the download function to use some of the earlier changes you made.

noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated

def _update_pip_packages(session: nox.Session) -> None:
session.run("pip-compile", "--generate-hashes", "--upgrade", "./requirements.in")
DEBUGPY_WHEEL_URLS = {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to check in these values rather than pull from here: https://pypi.org/pypi/debugpy/json

Copy link
Member

Choose a reason for hiding this comment

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

You could set it up to take the latest with 1.7.*. The same code is then reusable for the 2.7 and 3.6 cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, when I talked to Brett we decided to have the dict since debugpy didn't release new versions often.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. I would prefer to have the same noxfile across the 3 variants... and only have the version change between them

Copy link
Member

Choose a reason for hiding this comment

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

It depends on the security posture we want to take. If we are just doing this to make sure the download isn't corrupted or we didn't download the wrong file, then it's fine (although it does require an extra HTTP request). But if we are trying to avoid any potential PyPI compromise then static hash values mitigate that risk.

Copy link
Member

Choose a reason for hiding this comment

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

@karthiknadig what "3 variants" are you referring to?

And we can always store the information in a .json file if the key concern is just the code not changing instead of recording the hashes offline.

@paulacamargo25 paulacamargo25 force-pushed the platform-specific-vsix branch 2 times, most recently from 5a42ab8 to f03b5de Compare October 11, 2023 20:47
karthiknadig
karthiknadig previously approved these changes Oct 13, 2023
Copy link
Member

@brettcannon brettcannon left a comment

Choose a reason for hiding this comment

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

Missing some key pip install flags to avoid any supply-chain attacks.

build/azure-pipeline.pre-release.yml Outdated Show resolved Hide resolved
debugpy_info.json Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
noxfile.py Outdated Show resolved Hide resolved
@paulacamargo25 paulacamargo25 enabled auto-merge (squash) October 18, 2023 17:18
@paulacamargo25 paulacamargo25 removed the request for review from lszomoru October 18, 2023 20:14
noxfile.py Outdated
with url_lib.urlopen(value["url"]) as response:
data = response.read()
if (
hashlib.new(hash_algorithm, data).hexdigest()
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how you're using the hash algorithm as specified in the JSON data, and instead are just assuming it's always sha256. The reason to write it in the JSON that the hash is for sha256 is so you can get the hash algorithm from the JSON data and not have to hard-code that in the Python code.

session.log(f"Updating version from {package_json['version']} to {version}")
package_json["version"] = version
package_json_path.write_text(json.dumps(package_json, indent=4), encoding="utf-8")
def create_debugpy_json(session: nox.Session, version="1.7.0", cp="cp311"):
Copy link
Member

Choose a reason for hiding this comment

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

I don't see where this is called (maybe it's meant to be manual), but is there a reason to hard-code the Debugpy and Python version? It seems like something that should be stored in the JSON file so that this file requires no changes when we upgrade to a new Debugpy version or Python version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, i just wanted to have a default value just in case as , but i can remove this values. For other versions it would only be necessary to run the session with the parameters that we need through the terminal, this file will not need to be changed.

@paulacamargo25 paulacamargo25 merged commit 25425b5 into main Oct 18, 2023
18 checks passed
@paulacamargo25 paulacamargo25 deleted the platform-specific-vsix branch October 18, 2023 22:01
paulacamargo25 added a commit to paulacamargo25/vscode-python-debugger that referenced this pull request Oct 31, 2023
* Update nox file and pre release

* update args

* update value

* see params

* echo vars

* fix args

* fix args

* Fix error obj to string

* fix command line error

* update args

* update how access values

* Add args

* Send arguments

* tried ith arguments

* update value

* remove args extra

* update run

* send as env

* add to see other values

* update env variables

* use VSCETARGET

* fix variable

* Fix macOs error

* Fix lint in nox file

* Fix nox hash comparison

* Fix default value

* Remove unnecessary code

* Update platform url and debugpy version

* Fix dict key name

* Remove vsix for all platforms

* Fix pr comments

* fix download function

* fix hash code

* Read from json pypypackage

* add json file and session that creates it

* Reformat nox file

* resolve comments

* fix lint error

* update pipeline

* fix build error

* dont hardcode the hash
paulacamargo25 added a commit that referenced this pull request Jan 15, 2024
* update pre release (#71)

* Add port provider (#85)

* Add port attribute provider

* Add message

* Enabled proposed api

* Ignore all the ports

* Fix format

* fix pr comments

* Add regex for terminal command

* Add adapter to pattern

* Add global configuration for justMyCode (#91)

* Aff configuration

* Read just my code from settings too

* fix tests

* fix lint

* Update extension name (#84)

* Update package.json

* Fix readme

* update debug type name

* Fix tests

* Fix lint

* Fix test

* Update translations

* revert extension id

* Fix tests

* fix lint

* Update debug type name

* Fix translations and lint

* update debug type

* fix lint

* use version 1.7 (#95)

* Remove default justMyCode (#100)

* Fix error in workspace (#101)

* Fix error in workspace

* fix test and lint

* Fix format

* Bump mheap/github-action-required-labels from 3 to 5 (#23)

Bumps [mheap/github-action-required-labels](https://github.com/mheap/github-action-required-labels) from 3 to 5.
- [Release notes](https://github.com/mheap/github-action-required-labels/releases)
- [Commits](mheap/github-action-required-labels@v3...v5)

---
updated-dependencies:
- dependency-name: mheap/github-action-required-labels
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump @typescript-eslint/parser from 5.59.11 to 5.62.0 (#61)

Bumps [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint/tree/HEAD/packages/parser) from 5.59.11 to 5.62.0.
- [Release notes](https://github.com/typescript-eslint/typescript-eslint/releases)
- [Changelog](https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/parser/CHANGELOG.md)
- [Commits](https://github.com/typescript-eslint/typescript-eslint/commits/v5.62.0/packages/parser)

---
updated-dependencies:
- dependency-name: "@typescript-eslint/parser"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump typescript from 5.1.3 to 5.2.2 (#79)

Bumps [typescript](https://github.com/Microsoft/TypeScript) from 5.1.3 to 5.2.2.
- [Release notes](https://github.com/Microsoft/TypeScript/releases)
- [Commits](microsoft/TypeScript@v5.1.3...v5.2.2)

---
updated-dependencies:
- dependency-name: typescript
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Update npm packages (#105)

* update package.json

* Fux lint

* Update to node 18 (#106)

* Platform-specific vsix (#89)

* Update nox file and pre release

* update args

* update value

* see params

* echo vars

* fix args

* fix args

* Fix error obj to string

* fix command line error

* update args

* update how access values

* Add args

* Send arguments

* tried ith arguments

* update value

* remove args extra

* update run

* send as env

* add to see other values

* update env variables

* use VSCETARGET

* fix variable

* Fix macOs error

* Fix lint in nox file

* Fix nox hash comparison

* Fix default value

* Remove unnecessary code

* Update platform url and debugpy version

* Fix dict key name

* Remove vsix for all platforms

* Fix pr comments

* fix download function

* fix hash code

* Read from json pypypackage

* add json file and session that creates it

* Reformat nox file

* resolve comments

* fix lint error

* update pipeline

* fix build error

* dont hardcode the hash

* Fix GDPR comments (#113)

* Update readme (#111)

* Update readme

* Update README.md

Co-authored-by: Luciana Abud <[email protected]>

---------

Co-authored-by: Luciana Abud <[email protected]>

* Remove build folder from bundled vsix (#120)

* Update debugpy version

* Update nox and  pipelines

* update json file

* fix actions

* fix 3.6 error

* fix error running with 3.6

* fix lint

* update python version

* fix error in lint pipeline

* update pipelines

* fix nox file

* fix tests

* fix pipeline error

* fix lint

* use python 3.6

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Logan Ramos <[email protected]>
Co-authored-by: Luciana Abud <[email protected]>
Co-authored-by: Don Jayamanne <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debt Covers everything internal: CI, testing, refactoring of the codebase, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build platform specific VSIXs of debugpy
6 participants