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

Organize workflow #185

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

Pysis868
Copy link
Contributor

@Pysis868 Pysis868 commented Feb 10, 2023

How palatable are these changes, if even relevant now?


This change is Reviewable

Deduplicate and summarize workflow file.
@AppVeyorBot
Copy link

@billy1arm
Copy link
Member

@Meltie2013 Does this look ok to you ?

@AppVeyorBot
Copy link

@Meltie2013
Copy link
Contributor

@billy1arm Once I see an updated workflow with the changes in play, will make the call.

@AppVeyorBot
Copy link

Said up-to-date, not on a branch, had to `git pull origin master`.
@AppVeyorBot
Copy link

src/modules/Eluna Outdated Show resolved Hide resolved
src/realmd Outdated Show resolved Hide resolved
dep Outdated Show resolved Hide resolved
Copy link
Member

@billy1arm billy1arm left a comment

Choose a reason for hiding this comment

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

@Pysis868 can you revise this to the latest revision for the submodules ?

- dep
- Eluna
- realmd
- Extractor_projects

Strange each was in a detached HEAD state with a lot of changes / absent items.
Cleaned those / discarded all changes, then pulled for each directly because of that problem not allowing easy looping from git.

Already current:
- SD3
- win
@Pysis868
Copy link
Contributor Author

Pysis868 commented Aug 28, 2024

As of a270f22, and an extra win master ref set/checkout and pull, which provided no additional changes after committing:

> git submodule
 c02243ee29630c0d3b51f69d49589332927c9c0e dep (heads/master)
 5ca638f99fcd4a9f2cc94b42d0443ffa010618d0 src/modules/Eluna (heads/master)
 519bbc302878681abf25685a4f0e7df5db560bc9 src/modules/SD3 (heads/master)
 e63d3a3bd9f36497909e8e040a00055f205e7063 src/realmd (heads/master)
 1d533d5fec24fce8237e81d863f8270330363303 src/tools/Extractor_projects (heads/master)
 8c5fdae01893ad16af975a3eeee02585692452ac win (heads/master)

@AppVeyorBot
Copy link

For Eluna since it caused a `Hook.h` error.
to be consistent with master branch.
@Pysis868
Copy link
Contributor Author

Pysis868 commented Aug 28, 2024

Master was not using latest.
Thought this can all be solved with a merge, but I diffed and handled all manually now anyway:

git submodule status
 c02243ee29630c0d3b51f69d49589332927c9c0e dep (heads/master)
 058ffa1cb7f81bb111e3022d572a6311558c1d5f src/modules/Eluna (058ffa1)
 443cd7a5f701066cff624e3201f45de645f49cd2 src/modules/SD3 (heads/master-6-g443cd7a)
 e63d3a3bd9f36497909e8e040a00055f205e7063 src/realmd (heads/master)
 1d533d5fec24fce8237e81d863f8270330363303 src/tools/Extractor_projects (heads/master)
 8c5fdae01893ad16af975a3eeee02585692452ac win (heads/master)

@AppVeyorBot
Copy link

@AppVeyorBot
Copy link

@Pysis868
Copy link
Contributor Author

Pysis868 commented Aug 29, 2024

Run ./apps/ci/ci-packages.sh
  ./apps/ci/ci-packages.sh
  shell: /usr/bin/bash -e {0}
/home/runner/work/_temp/870baad1-32d8-40b1-9a44-fa551d09bc65.sh: line 1: ./apps/ci/ci-packages.sh: Permission denied
Error: Process completed with exit code 126.

apps/ci/ci-compiler-update.sh uses sudo, so thought it would be fine for this package script too. The workflow simply runs the scripts, and thought it was working before as well, since there was a build folder fix added.

Does added the source command prefix really make a difference?

Copy link
Contributor

@Meltie2013 Meltie2013 left a comment

Choose a reason for hiding this comment

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

@Pysis868 Please check comment for details.

Add `source` to package script invocation.
Bring back commands for Windows build without script execution capability.
@AppVeyorBot
Copy link

Copy link
Contributor Author

@Pysis868 Pysis868 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 4 files at r1, 1 of 6 files at r2, 1 of 1 files at r3, 3 of 4 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @billy1arm and @Meltie2013)


.github/workflows/core_build.yml line 21 at r6 (raw file):

Previously, Meltie2013 wrote…

This should be:
run: source ./apps/ci/ci-packages.sh

Done.

Copy link
Contributor Author

@Pysis868 Pysis868 left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @billy1arm and @Meltie2013)


apps/ci/ci-compile.sh line 0 at r2 (raw file):

Previously, Meltie2013 wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

This should remain to see if the directory exists already or not.

Done.


src/realmd line 0 at r4 (raw file):

Previously, billy1arm (Antz) wrote…

@Pysis868 can you revise this to the latest revision

Done.


dep line 0 at r4 (raw file):

Previously, billy1arm (Antz) wrote…

@Pysis868 can you revise this to the latest revision

Done.


.github/workflows/core_windows_build.yml line 29 at r6 (raw file):

Previously, Meltie2013 wrote…

This line should be reverted back to how it was, since Shell files can not run in Windows directly.

Done.


src/modules/Eluna line 0 at r4 (raw file):

Previously, billy1arm (Antz) wrote…

@Pysis868 can you revise this to the latest revision

Done.


src/tools/Extractor_projects line 0 at r4 (raw file):

Previously, billy1arm (Antz) wrote…

@Pysis868 can you revise this to the latest revision

Done.

Copy link
Contributor Author

@Pysis868 Pysis868 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @billy1arm and @Meltie2013)

Copy link
Contributor Author

@Pysis868 Pysis868 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @billy1arm and @Meltie2013)

Copy link
Member

@billy1arm billy1arm left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r1, 1 of 1 files at r3, 3 of 4 files at r5, 2 of 2 files at r6, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @Meltie2013)

@AppVeyorBot
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants