-
Notifications
You must be signed in to change notification settings - Fork 80
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
Update nixpkgs_cc_configure
to work without nix_file
or nix_content
#334
Conversation
01a523c
to
f5b838b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
toolchains/cc/cc.bzl
Outdated
"--argstr", | ||
"ccType", | ||
"ccTypeExpression", | ||
"--arg", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this would be a bit simpler?
@@ -388,17 +388,11 @@ def nixpkgs_cc_configure(
nix_file_deps.append(nix_file)
elif nix_file_content:
nix_expr = nix_file_content
-
- if attribute_path and nix_expr == None:
- nixopts.extend([
- "--argstr",
- "ccType",
- "ccTypeExpression",
- "--arg",
- "ccExpr",
- "(import <nixpkgs> {{}}).{0}".format(attribute_path),
- ])
elif attribute_path:
+ nix_expr = "(import <nixpkgs> {{}}).{0}".format(attribute_path)
+ attribute_path = None
+
+ if attribute_path:
nixopts.extend([
"--argstr",
"ccType",
I think it's equivalent but the logic is a little convoluted so I may be wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure how to make a GitHub suggested change that I've seen people do sometimes but I made a commit.
(Edit: apparently you can't add suggestions on deleted lines 😞)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion! I've added the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good change. Just one suggestion about the logic around nix_expr
and attribute_path
.
The simple example for the cc toolchain was updated to showcase the usage. I'm not sure if this should have a separate test altogether.
Related issue: #328