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

Fix mingw build #283

Closed
wants to merge 4 commits into from
Closed

Fix mingw build #283

wants to merge 4 commits into from

Conversation

cielavenir
Copy link
Contributor

  • After c183961 , make -f Makefile.unx programs/igzip arch=mingw host_cpu=x86_64 fails without b886279 .
  • Even then, make -f Makefile.unx programs/igzip arch=mingw host_cpu=base_aliases CC=i686-w64-mingw32-gcc fails, which is fixed by 13272ef .

@cielavenir
Copy link
Contributor Author

@pablodelara should I add mingw CI? (Adding macos-14 was easy as I used macos-13 template, but...)

@cielavenir
Copy link
Contributor Author

@pablodelara added mingw CI. previously there were no Windows CI, so good?

@cielavenir
Copy link
Contributor Author

@pablodelara could you merge this?

@pablodelara
Copy link
Contributor

@pablodelara could you merge this?

Could you rebase on top of latest "master" branch? So only your commit show up.

@cielavenir
Copy link
Contributor Author

@pablodelara rebased.

@pablodelara
Copy link
Contributor

Thanks @cielavenir. Looks like there are some errors in the test log, could you take a look at them? Thanks!

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@cielavenir
Copy link
Contributor Author

@pablodelara Now I need to discuss one thing.

https://github.com/intel/isa-l/blob/master/make.inc#L203

have_threads ?= $(shell printf "int main(void){return 0;}\n" | $(CC) -x c - -o /dev/null -lpthread && echo y )

Running this code on mingw is incorrect. However, although isa-l can be built on mingw, saying that mingw is unsupported only due to this line is quite wasteful (もったいない) for me.

How this can be improved?


If I should make a separate issue, please tell so.

@pablodelara
Copy link
Contributor

Could you add a new commit commenting out that line to see if that solves all issues? Thanks!

@cielavenir
Copy link
Contributor Author

@pablodelara could you check current workflow result again?

@pablodelara
Copy link
Contributor

pablodelara commented Nov 19, 2024

@pablodelara could you check current workflow result again?

I did and there were some errors that were not actually flagged by the Github runner job, like this one:
make: *** [make.inc:236: erasure_code_perf.run] Error -1073740940

This was another issue that has been fixed already, but actually we should improve the check for exit codes in CI (I left a comment).
Many thanks for the time you are taking to improve/fix this.

- name: Build
shell: bash
run: |
make -j $(nproc) -f Makefile.unx programs/igzip tests SIM= arch=mingw host_cpu=x86_64 AR=x86_64-w64-mingw32-gcc-ar
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add "|| exit /b 1" at the end of these command lines? The same way it is done in the current ci.yml on the windows build (run_tests_windows)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @cielavenir, did you try adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pablodelara with shell: bash, intermediate error will halt the entire script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for looking into this, @cielavenir, Much appreciated!

Signed-off-by: Taiju Yamada <[email protected]>
Signed-off-by: Taiju Yamada <[email protected]>
@cielavenir
Copy link
Contributor Author

@pablodelara actually what I need instead is to force bash (msys)

done

@pablodelara
Copy link
Contributor

This is merged now, thanks for the work!

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