Skip to content
This repository has been archived by the owner on Aug 7, 2023. It is now read-only.

ci: use pulsar in ci #39

Merged
merged 5 commits into from
Jul 11, 2023
Merged

Conversation

scagood
Copy link
Contributor

@scagood scagood commented Jul 7, 2023

No description provided.

Copy link
Contributor Author

@scagood scagood left a comment

Choose a reason for hiding this comment

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

The main thing I am unsure of is how to go about fixing the windows stuff 🤔

I figured I'd put the progress of mac + linux at least passing here though

# ./node_modules/.bin/atom-package-deps .
pulsar --package install
npx atom-package-deps .
pulsar --package install linter-eslint
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no idea why this install does not work 😬

Copy link
Member

Choose a reason for hiding this comment

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

I think on Windows you've still got to use ppm instead of pulsar --package, so give that a shot. If that doesn't work, I'll summon another person from Pulsar core to help troubleshoot.

patches/atom-package-deps+8.0.0.patch Outdated Show resolved Hide resolved
.github/workflows/CI.yml Show resolved Hide resolved
apm install
apm install linter-eslint
# ./node_modules/.bin/atom-package-deps .
pulsar --package install
Copy link
Contributor Author

@scagood scagood Jul 7, 2023

Choose a reason for hiding this comment

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

I initially tried to use ppm here, but it failed on macos.

I found that its disabled here, not sure why:
https://github.com/pulsar-edit/action-pulsar-dependency/blob/main/action.yml#L27

Copy link
Member

Choose a reason for hiding this comment

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

@confused-Techie, do you remember why? ^^

Choose a reason for hiding this comment

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

This should only be commented out, in case someone wanted to symlink ppm, but the idea is we move fully over to using pulsar -p.

On a side note, @savetheclocktower is 100% right, you must still use apm on Windows. Plus the installer doesn't add anything to the path. You could run the tests from the path of the package after install, like I do here, when running on windows

Copy link
Contributor Author

@scagood scagood Jul 8, 2023

Choose a reason for hiding this comment

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

mmm, I ended up using a patched version atom-package-deps to install the packages on windows 🤔
For reference, both ppm, and pulsar -p did not install the package
I probably need to work out a better solution 😬

@savetheclocktower
Copy link
Member

Thank you so very much for doing this.

Looks like you got Windows working — are we good to land this PR?

@savetheclocktower
Copy link
Member

Ah, I see some of the tests are still skipped. Never mind.

@scagood
Copy link
Contributor Author

scagood commented Jul 7, 2023

This is just because the ppm install linter-eslint just does not seem to install linter-eslint. So the tests all throw an error saying as much.

See: https://github.com/scagood/linter-eslint-node/actions/runs/5484423520/jobs/9991948934

@savetheclocktower
Copy link
Member

Maybe try installing from GitHub just for fun?

ppm install AtomLinter/linter-eslint

@scagood

This comment was marked as outdated.

@scagood

This comment was marked as outdated.

@savetheclocktower
Copy link
Member

Oh, the package path in package-integration-spec.js is still hard-coded:

const packagesRoot = Path.join(root, '.atom', 'packages');

Try switching that to .pulsar.

@scagood

This comment was marked as outdated.

@scagood

This comment was marked as off-topic.

@scagood

This comment was marked as outdated.

@scagood
Copy link
Contributor Author

scagood commented Jul 8, 2023

@savetheclocktower What do you think of the hack for installing deps on windows?

If its reasonable we could look to merge this. May see if I can get a windows machine and also try to fix ppm/pulsar -p

@scagood scagood marked this pull request as ready for review July 8, 2023 13:43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: Create a package for this (probably called pulsar-package-deps)

Maybe we should put that in the pulsar org 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Or you could start the pulsar-community org. I could transfer any packages from atom-community and AtomLinter that you want.

Copy link
Member

Choose a reason for hiding this comment

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

@confused-Techie, once again I summon thee: does pulsar-package-deps make sense to be hosted by the Pulsar org, or should it be somewhere else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pulsar-package-deps changes found in this commit -- pulsar-linter/package-deps@0bad339

Let me know what we want doing with it.

I am also considering bumping all the package version see: https://github.com/steelbrain/package-deps/pulls

@scagood
Copy link
Contributor Author

scagood commented Jul 11, 2023

@savetheclocktower Should we progress this PR?

@scagood scagood changed the title ci: attempt to use pulsar in ci ci: use pulsar in ci Jul 11, 2023
@savetheclocktower
Copy link
Member

Looks like we're good. I'll land this and then you can rebase #38 and #41. Thanks a lot!

Another thing we should tackle in the future is automatic release to the Pulsar Package Repository, but that might require some small amount of @UziTech's time just to work out how to generate and store a secret token. It can wait.

@savetheclocktower savetheclocktower merged commit d6587c5 into AtomLinter:main Jul 11, 2023
6 checks passed
@scagood scagood deleted the pulsar-ci branch July 19, 2023 08:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants