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

Add SBNF #8826

Merged
merged 1 commit into from
Dec 16, 2023
Merged

Add SBNF #8826

merged 1 commit into from
Dec 16, 2023

Conversation

BenjaminSchaaf
Copy link
Contributor

  • I'm the package's author and/or maintainer.
  • I have have read [the docs][1].
  • I have tagged a release with a [semver][2] version number.
  • My package repo has a description and a README describing what it's for and how to use it.
  • My package doesn't add context menu entries. *
  • My package doesn't add key bindings. **
  • Any commands are available via the command palette.
  • Preferences and keybindings (if any) are listed in the menu and the command palette, and open in split view.
  • If my package is a syntax it doesn't also add a color scheme. ***
  • If my package is a syntax it is named after the language it supports (without suffixes like "syntax" or "highlighting").
  • I use [.gitattributes][3] to exclude files from the package: images, test files, sublime-project/workspace.

My package is SBNF

I'm not 100% sure if the older tags are a problem. The newest tag and all future ones are on a different orphan branch that includes binaries.

There are no packages like it in Package Control.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: ERROR

Repo link: SBNF
Results help

Packages added:
  - SBNF

Processing package "SBNF"
  - ERROR: '.sublime-package' files have no business being inside a package
    - File: .sublime-package
  - WARNING: The package does not contain a top-level LICENSE file. A license helps users to contribute to the package.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: WARNING

Repo link: SBNF
Results help

Packages added:
  - SBNF

Processing package "SBNF"
  - WARNING: '.no-sublime-package' is defined. Please verify that it is *really* necessary
  - WARNING: The package does not contain a top-level LICENSE file. A license helps users to contribute to the package.

@deathaxe
Copy link
Contributor

deathaxe commented Oct 7, 2023

Directly pushing binaries to a tag seems rather uncommon.

Wouldn't it be more obvious to create a dedicated download artefact and upload it for each release?

This way each release could contain a SBNF.sublime-package and maybe binaries for direct use?


The package itself works, well, except I wouldn't use start on Windows.

see: https://github.com/BenjaminSchaaf/sbnf/blob/7ecb423081a1bea2209a3906460f2ae0bed13987/sbnf-windows-universal.bat#L4

It has 2 side effects:

  1. A command line window pops up while build is running
  2. error messages are not caught by ST, thus no syntax issues are displayed via annotations

@deathaxe
Copy link
Contributor

deathaxe commented Oct 7, 2023

A readme, describing the package and maybe a license file should be specified, so packagecontrol.io can display a description of what the package does.

URL of a README can be specified explicitly in repository.json or if present is used from the tag's commit.

@BenjaminSchaaf
Copy link
Contributor Author

Directly pushing binaries to a tag seems rather uncommon.

Wouldn't it be more obvious to create a dedicated download artefact and upload it for each release?

This way each release could contain a SBNF.sublime-package and maybe binaries for direct use?

Not sure what you mean by this. Are you suggesting SBNF include a python plugin that downloads the binaries?

@deathaxe
Copy link
Contributor

deathaxe commented Oct 8, 2023

What I mean is to let a tag point to a normal commit on master branch and deploy a downloadable asset in related release, instead of pushing compiled binaries to a separate package branch.

Example:

It would basically mean to extend the github workflow to create a zip file (or sublime-package) with all the package content (binaries, syntax files, ... - which is currently pushed to package branch) and upload it to the release using actions/upload-artifact.

      # - compile binaries
      # - pack everything to SBNF.zip or SBNF.sublime-package
       
      - uses: actions/upload-artifact@v3
        with:
          path: |
            *.sublime-package

This way it keeps pretty obvious which commit a tag/release belongs to, which might not be too obvious, with current strategy.

TBH, I just stumbled over Source code (zip) being the only available download asset in https://github.com/BenjaminSchaaf/sbnf/releases/tag/0.6.3 and was confused by that file containing compiled binaries.

@BenjaminSchaaf
Copy link
Contributor Author

Right, how can that be hooked up to package control?

@deathaxe
Copy link
Contributor

deathaxe commented Oct 8, 2023

Well, I totally forgot about wbond/package_control#1484 as "easy to use" solution for Bitbucket/Github/Gitlab deployed packages.

A common practice I've seen so far is to manage a packages.json (or repository.json), which points to an explicit release. It would need to be updated as part of a release/deploy workflow and should be committed back to e.g. master branch when release is done, to make it availabe via universally available URL, which can be included into default channel.

Actually Package Control is deployed this way.

the package in package.json

https://github.com/SublimeText/wbond-packages/blob/29df220823f83493b4b0dd581930493a20865422/packages.json#L15-L36

the include in channel.json

https://github.com/wbond/package_control_channel/blob/cf7200920a9586dc3f951f03f0b853ae61a78237/channel.json#L105C3-L105C3


Well, finally your current approach is probably easier and not that cumbersome with regards to usability.

@BenjaminSchaaf
Copy link
Contributor Author

I've made a repository and am uploading artifacts as part of gh releases now. Adding a custom repository works and installing the package succeeds. Only issue is that package control isn't extracting the executable flag, so my executables/shell scripts aren't working.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: ERROR

Results help

Repositories added:
  - https://raw.githubusercontent.com/BenjaminSchaaf/sbnf/master/package_control/packages.json

Processing repository "https://raw.githubusercontent.com/BenjaminSchaaf/sbnf/master/package_control/packages.json"
  - Found 1 package

Packages added:
  - SBNF

Processing package "SBNF"
  - ERROR: fetching package HTTP error 302 downloading https://github.com/BenjaminSchaaf/sbnf/releases/download/0.6.4/SBNF.sublime-package.

@deathaxe
Copy link
Contributor

deathaxe commented Oct 9, 2023

With regards to executable flag:

  1. I wonder how SFTP handles it, as it also ships with various binaries, at least on Windows.
  2. It's not only Package Control, but also python's ZipFile, which doesn't extract file flags, actually.

It's possible to add that, however would it work for those using a future release of PC, only.

@deathaxe
Copy link
Contributor

deathaxe commented Oct 9, 2023

It appears package reviewer bot doesn't handle redirects.

Release assets are redirected to something like:

https://objects.githubusercontent.com/github-production-release-asset-2e65be/241113847/184a3ffb-c6bb-4ac8-90af-f8fdba8785aa?X-Amz-Algorithm=AWS4-HMAC-SHA256&X-Amz-Credential=AKIAIWNJYAX4CSVEH53A%2F20231009%2Fus-east-1%2Fs3%2Faws4_request&X-Amz-Date=20231009T154502Z&X-Amz-Expires=300&X-Amz-Signature=a5d725cf95cdb6b506e603d030843f43181388e7daf8f789ee37bfa02a7a2dc9&X-Amz-SignedHeaders=host&actor_id=0&key_id=0&repo_id=241113847&response-content-disposition=attachment%3B%20filename%3DSBNF.sublime-package&response-content-type=application%2Foctet-stream

Despite that limitiation, I can confirm adding the repository.json locally, enables installation of SBNF without issues.

@rchl
Copy link
Contributor

rchl commented Oct 9, 2023

Have you tried PC3? I recall this (the redirect handling) being a blocking issue for some packages.

@deathaxe
Copy link
Contributor

deathaxe commented Oct 9, 2023

Yes, PC3.4.1, Windows 11, using urllib and wininet downloader. I also didn't find evidents for it not working when I was looking into wbond/package_control#1502.

wininet is used on Windows, by default. urllib on Linux/MacOS.

Redirects are a default "handler" in urllib, wininet handles it transparently.

I haven't checked curl, wget and osscrypto downloaders.

packagecontrol.io doesn't download or handle packages and thus - even if it suffers - shouldn't realize it.

I agree, however it not being an ideal and confusing circumstance.

@BenjaminSchaaf
Copy link
Contributor Author

I've submitted patches to both package control 3 and 4 to extract file flags, in testing both were able to install SBNF with the right flags on Linux.

@deathaxe
Copy link
Contributor

deathaxe commented Dec 3, 2023

Now that PC4.0 is out, which restores executable bits, it's safe to merge this.

Copy link
Collaborator

@packagecontrol-bot packagecontrol-bot left a comment

Choose a reason for hiding this comment

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

Automated testing result: ERROR

Results help

Repositories added:
  - https://raw.githubusercontent.com/BenjaminSchaaf/sbnf/master/package_control/packages.json

Processing repository "https://raw.githubusercontent.com/BenjaminSchaaf/sbnf/master/package_control/packages.json"
  - Found 1 package

Packages added:
  - SBNF

Processing package "SBNF"
  - ERROR: fetching package HTTP error 302 downloading https://github.com/BenjaminSchaaf/sbnf/releases/download/0.6.4/SBNF.sublime-package.

@braver
Copy link
Collaborator

braver commented Dec 16, 2023

@deathaxe just to be sure, can we ignore the 302 here?

@deathaxe
Copy link
Contributor

Yes, we can.

@braver braver merged commit 04b3d33 into wbond:master Dec 16, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants