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

use older os to compiler so dynamic libraries work better #36

Open
wants to merge 2 commits into
base: z80
Choose a base branch
from

Conversation

mateoconlechuga
Copy link

See relevant toolchain issue here: CE-Programming/toolchain#439

@adriweb
Copy link

adriweb commented May 22, 2023

Hmm can jacobly launch CI on this or something, to see if it builds as expected?

@mateoconlechuga
Copy link
Author

Hmm can jacobly launch CI on this or something, to see if it builds as expected?

It already built fine on my fork: https://github.com/mateoconlechuga/llvm-project/actions/runs/5039720221/jobs/9038047940

@adriweb
Copy link

adriweb commented May 22, 2023

Ah great, merge time then 👀

@mateoconlechuga
Copy link
Author

waits 3 weeks for the merge

@mateoconlechuga
Copy link
Author

Bump bump 😃

@mateoconlechuga
Copy link
Author

I'm going to fork the project at this point :(

Copy link

@adriweb adriweb left a comment

Choose a reason for hiding this comment

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

Ok so I propose some modifications to just add the older OSes instead, since @jacobly0 said this (IRC).

06:06:40 <@jacobly> I have no problem with adding build configurations, but I'm not removing the existing ones

I guess now they have to be accepted so the PR gets updated, then jacobly can merge it.

@@ -10,7 +10,7 @@ jobs:
strategy:
fail-fast: false
matrix:
runs-on: [ubuntu-latest, macos-latest, windows-latest]
runs-on: [ubuntu-20.04, macos-11, windows-latest]
Copy link

Choose a reason for hiding this comment

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

Suggested change
runs-on: [ubuntu-20.04, macos-11, windows-latest]
runs-on: [ubuntu-20.04, ubuntu-latest, macos-11, macos-latest, windows-latest]

@@ -155,7 +155,7 @@ jobs:
strategy:
fail-fast: false
matrix:
runs-on: [ubuntu-latest, macos-latest, windows-latest]
runs-on: [ubuntu-20.04, macos-11, windows-latest]
Copy link

Choose a reason for hiding this comment

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

Suggested change
runs-on: [ubuntu-20.04, macos-11, windows-latest]
runs-on: [ubuntu-20.04, ubuntu-latest, macos-11, macos-latest, windows-latest]

@@ -280,7 +280,7 @@ jobs:
strategy:
fail-fast: false
matrix:
runs-on: [ubuntu-latest, macos-latest, windows-latest]
runs-on: [ubuntu-20.04, macos-11, windows-latest]
Copy link

Choose a reason for hiding this comment

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

Suggested change
runs-on: [ubuntu-20.04, macos-11, windows-latest]
runs-on: [ubuntu-20.04, ubuntu-latest, macos-11, macos-latest, windows-latest]

@@ -332,7 +332,7 @@ jobs:
strategy:
fail-fast: false
matrix:
runs-on: [ubuntu-latest, macos-latest, windows-latest]
runs-on: [ubuntu-20.04, macos-11, windows-latest]
Copy link

Choose a reason for hiding this comment

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

Suggested change
runs-on: [ubuntu-20.04, macos-11, windows-latest]
runs-on: [ubuntu-20.04, ubuntu-latest, macos-11, macos-latest, windows-latest]

@@ -382,7 +382,7 @@ jobs:
strategy:
fail-fast: false
matrix:
runs-on: [ubuntu-latest, macos-latest, windows-latest]
runs-on: [ubuntu-20.04, macos-11, windows-latest]
Copy link

Choose a reason for hiding this comment

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

Suggested change
runs-on: [ubuntu-20.04, macos-11, windows-latest]
runs-on: [ubuntu-20.04, ubuntu-latest, macos-11, macos-latest, windows-latest]

@adriweb
Copy link

adriweb commented Jun 16, 2023

Well in addition to that now, I suppose we have to use ${{matrix.runs-on}} instead of ${{runner.os}} in the various named resources, all good with that?

@adriweb
Copy link

adriweb commented Jun 20, 2023

Easier solution, regarding my last comment, no need to change the names, we can just upload the binaries if the currently running VM is the version we want. The other (newer) VMs will just continue to be there to test the build, that's fine.

(23:32:18 <@jacobly> I don't care about the artifact names, feel free to change the backwards compatible names to now be the old versions)

@adriweb
Copy link

adriweb commented Jun 22, 2023

Well I put all the names differently for now, jacobly is able to merge it if it's ok https://github.com/adriweb/llvm-project/actions/runs/5335820947
Then we just have to make the toolchain use those names when downloading files in its CI.

@mateoconlechuga
Copy link
Author

I'm going to fork this into CE-Programming since the PR doesn't seem to want to be merged :(

@adriweb
Copy link

adriweb commented Jun 27, 2023

Well he said he'll do it so :P
(but when?)

Also take the commit from my fork I guess, if you really want to do that.

@mateoconlechuga
Copy link
Author

I guess I can wait a bit longer :)

@adriweb
Copy link

adriweb commented Jul 4, 2023

So, I did the thing to upload selected artifacts to a "nightly" pre-release like in CEmu or the toolchain, it works fine, as we can see in the release files :)
So if this is merged too, there won't be a need to depend on jacobly's server script to fetch artifacts, but most importantly, we don't have the 90-day binary retention limit issues.

@adriweb
Copy link

adriweb commented Jul 15, 2024

This can be closed, it was actually done in 5f8512f

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

Successfully merging this pull request may close these issues.

2 participants