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

Switch git submodules to Wally packages #584

Merged
merged 16 commits into from
Aug 3, 2022

Conversation

boatbomber
Copy link
Member

@boatbomber boatbomber commented Jul 20, 2022

Removes the git submodules and moves to a more elegant solution with Wally.
Also migrates to @evaera's Promise. No compatibility issues arose, and plugin tests all pass.

@LPGhatguy LPGhatguy closed this Jul 22, 2022
@LPGhatguy LPGhatguy reopened this Jul 22, 2022
@boatbomber boatbomber marked this pull request as ready for review July 22, 2022 04:59
Copy link

@mkargus mkargus left a comment

Choose a reason for hiding this comment

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

Everything looks good.

One minor suggestion, remove the step for submodules on the workflows since the plugin part was the only one needing it:

  uses: action/checkout@v3
- with:
-    submodules: true

Copy link
Contributor

@imacodr imacodr left a comment

Choose a reason for hiding this comment

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

Everything looks good for release to me.

@boatbomber boatbomber added type: tech debt Internal work that needs to happen status: needs review This work is mostly done, but just needs work to integrate and merge it. labels Jul 25, 2022
@@ -2,7 +2,8 @@
Defines the errors that can be returned by the reconciler.
]]

local Fmt = require(script.Parent.Parent.Parent.Fmt)
local Packages = script.Parent.Parent.Parent.Packages
Copy link
Contributor

Choose a reason for hiding this comment

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

This kinda line is why we use FindFirstAncestor("Rojo") a lot. 😅

Copy link
Contributor

@LPGhatguy LPGhatguy left a comment

Choose a reason for hiding this comment

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

Thank you, this is a great migration!

@LPGhatguy LPGhatguy merged commit e864cf0 into rojo-rbx:master Aug 3, 2022
@boatbomber boatbomber deleted the wally-package-management branch August 3, 2022 23:00
Kampfkarren pushed a commit that referenced this pull request May 26, 2023
Alright, so I hate to be the one to do this, but #584 broke crates.io
publishing and also caused librojo to be unusable. I see that there was
some discussion on Discord shortly after the problem was realized, but
there was no action taken.

I think keeping librojo and publishing working far, far outweigh any
convenience added by Wally.

I've kept the same `Packages` naming convention to keep the diff
minimal.
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
* Switch git submodules to Wally packages

* Update build snapshot

* Add wally to foreman and use latest versions

* Install packages in CI runners

* Fix indents

* Install packages in the correct directory

* Install packages in correct dir of release action too

* Remove submodules from ci checkout

* Remove submodules from release checkout

* Update selene with latest fix

* Fix whitespace

Co-authored-by: Lucien Greathouse <[email protected]>
Dekkonot pushed a commit to UpliftGames/rojo that referenced this pull request Jan 11, 2024
Alright, so I hate to be the one to do this, but rojo-rbx#584 broke crates.io
publishing and also caused librojo to be unusable. I see that there was
some discussion on Discord shortly after the problem was realized, but
there was no action taken.

I think keeping librojo and publishing working far, far outweigh any
convenience added by Wally.

I've kept the same `Packages` naming convention to keep the diff
minimal.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: needs review This work is mostly done, but just needs work to integrate and merge it. type: tech debt Internal work that needs to happen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants