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

Aro translate-c #17221

Merged
merged 5 commits into from
Oct 2, 2023
Merged

Aro translate-c #17221

merged 5 commits into from
Oct 2, 2023

Conversation

Vexu
Copy link
Member

@Vexu Vexu commented Sep 21, 2023

This starts work on #16268. None of it is tested currently so I'll need to update the test harness before this is contributor friendly.

@Vexu Vexu force-pushed the aro-translate-c branch 4 times, most recently from 5da43fb to 60ecbf3 Compare September 21, 2023 20:16
@kubkon
Copy link
Member

kubkon commented Sep 22, 2023

Very exciting!

@andrewrk
Copy link
Member

andrewrk commented Sep 26, 2023

For maintaining Aro, I think we should pick one of these options:

  • Put Aro in src/, don't mark it as vendored, allow tight coupling with other compiler code, and periodically port code back and forth between the Zig source tree and the Aro source tree. This is @kubkon's current strategy for zld.
  • Start maintaining Aro in the zig source tree itself; archive the external Aro project.
  • Merge Support relative paths in package manager #14603, continue maintaining Aro separately, use deps/, mark the files as vendored, have a policy of no downstream patches, and use Aro via @import("aro") using the public Aro API only.

Up to you, @Vexu. What do you prefer?

@Vexu
Copy link
Member Author

Vexu commented Sep 26, 2023

I was planning on going with option 3.

@andrewrk
Copy link
Member

Sounds great 👍

@andrewrk
Copy link
Member

So, what's the transition plan? For now, enable it only when -Denable-llvm=false?

@Vexu
Copy link
Member Author

Vexu commented Sep 29, 2023

That's what it does currently:

// Make a decision on whether to use Clang or Aro for translate-c and compiling C files.
const c_frontend = blk: {
    if (!build_options.have_llvm) break :blk .aro;
    if (options.c_frontend) |explicit| break :blk explicit;
    break :blk .clang;
};

@Vexu Vexu force-pushed the aro-translate-c branch 2 times, most recently from 1ef554f to 1d4e9bc Compare September 29, 2023 10:08
@Vexu
Copy link
Member Author

Vexu commented Sep 29, 2023

Not sure what the CI failure is caused by. I can't reproduce it locally with zig build.

@ianprime0509
Copy link
Contributor

I believe the CI failure is because zig2 doesn't have any of the package management code included in it, including path-based dependencies (

zig/src/main.zig

Lines 4798 to 4803 in 3031819

const deps_pkg = try Package.createFilePkg(gpa, local_cache_directory, "dependencies.zig",
\\pub const packages = struct {};
\\pub const root_deps: []const struct { []const u8, []const u8 } = &.{};
\\
);
try main_pkg.add(gpa, "@dependencies", deps_pkg);
). I can also reproduce this when building from scratch starting from CMake.

@andrewrk
Copy link
Member

andrewrk commented Oct 1, 2023

Yes I'm sorry, after I mentioned #14603 above, I realized it wasn't actually going to work exactly that way because we want to avoid all that machinery while bootstrapping. I think it will be fine just adding logic to build.zig instead for integration. I think you had it that way in the first place anyway.

@Vexu Vexu force-pushed the aro-translate-c branch from 1d4e9bc to ad4c72e Compare October 1, 2023 08:46
Copy link
Member

@andrewrk andrewrk 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 great and ready to merge, except I have one request-

src/main.zig Outdated
Comment on lines 442 to 443
\\ -fclang Force using Clang as the C/C++ compilation backend
\\ -fno-clang Prevent using Clang as the C/C++ compilation backend
Copy link
Member

Choose a reason for hiding this comment

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

I think using CFrontend internally is a good idea, however, as for the CLI, I don't think zig ever plans to have more options for C frontends. The plan is:

  • (now) support clang and aro
  • (future) support aro only

I think -fclang and -fno-clang is nice since it is very similar to the llvm and lld flags above, and all 3 sets of flags will disappear together one day.

Do you have a strong preference for -c-frontend? I would prefer keeping the CLI as -fclang and -fno-clang.

Copy link
Member Author

Choose a reason for hiding this comment

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

No preference, -fclang/-fno-clang works for me.

Copy link
Member

Choose a reason for hiding this comment

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

Nice! In that case, feel free to proceed with the merge after making that change 👍

@Vexu Vexu force-pushed the aro-translate-c branch from ad4c72e to 5418fda Compare October 1, 2023 20:50
@Vexu Vexu force-pushed the aro-translate-c branch from 5418fda to 5792570 Compare October 1, 2023 20:52
@Vexu Vexu enabled auto-merge October 1, 2023 20:54
@Vexu Vexu merged commit fc4d53e into ziglang:master Oct 2, 2023
@Vexu Vexu deleted the aro-translate-c branch October 2, 2023 11:48
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.

4 participants