Skip to content

Fix: assertion when using zig toolchain #162

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

Merged

Conversation

nosamad
Copy link
Contributor

@nosamad nosamad commented Jan 24, 2024

The zig toolchain uses the file extension .obj instead of .o

The zig toolchain uses the file extension .obj instead of .o
replace split with rsplit to retrieve the correct Makefile target, when absolute windows paths are used
@cpsauer
Copy link
Contributor

cpsauer commented Jan 27, 2024

Hello again @nosamad! Definitely down to merge. Couple quick questions:

  1. For my learning, what changed your thinking vs last time? (I thought zig had been respecting Bazel's -o after all?) A windows case?
  2. For the split, maybe we should split on ': ', in case there are multiple absolute windows paths?

Thanks so much for working to leave things better than you found them :)
Chris

@cpsauer
Copy link
Contributor

cpsauer commented Jan 30, 2024

^ Went ahead and did that--I'll merge, but would still love it if you'd reply when you get a chance!

@cpsauer cpsauer merged commit c4918fa into hedronvision:main Jan 30, 2024
@nosamad
Copy link
Contributor Author

nosamad commented Jan 30, 2024

Hello again @nosamad! Definitely down to merge. Couple quick questions:

  1. For my learning, what changed your thinking vs last time? (I thought zig had been respecting Bazel's -o after all?) A windows case?
    I think it is a windows case. As far as I know, there is no option of defining the Artefact File-Extension through a toolchain definition - but I didn't review what they do, so maybe there is a possibility.
  2. For the split, maybe we should split on ': ', in case there are multiple absolute windows paths?
    I don't know if this approach would be sufficient, as dep-Files are valid Makefiles and theoretically every valid Makefile Expression is allowed in the context of targets and prerequisites of a rule - which also can be defined through the command line (gcc). Does a simple Makefile-Parser exist in python? On the other hand your "simple" approach works in most cases :)

Thanks so much for working to leave things better than you found them :)
Actually thank you for your nice work!
Chris
Sorry for the late response.

@nosamad
Copy link
Contributor Author

nosamad commented Jan 30, 2024

Hello again @nosamad! Definitely down to merge. Couple quick questions:

  1. For my learning, what changed your thinking vs last time? (I thought zig had been respecting Bazel's -o after all?) A windows case?
    I think it is a windows case. As far as I know, there is no option of defining the Artefact File-Extension through a toolchain definition - but I didn't review what they do, so maybe there is a possibility.
  2. For the split, maybe we should split on ': ', in case there are multiple absolute windows paths?
    I don't know if this approach would be sufficient, as dep-Files are valid Makefiles and theoretically every valid Makefile Expression is allowed in the context of targets and prerequisites of a rule - which also can be defined through the command line (gcc). Does a simple Makefile-Parser exist in python? On the other hand your "simple" approach works in most cases :)

Thanks so much for working to leave things better than you found them :)
Actually thank you for your nice work!
Chris
Sorry for the late response.

Sorry I was so in a hurry that I missed your main points. Are you calling the compiler for depfile creation? I will investigate further, but it will take some time. Maybe we should provide some integration tests?

Thanks

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