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

Update nixpkgs_cc_configure to work without nix_file or nix_content #334

Merged
merged 1 commit into from
Feb 22, 2023

Conversation

z8v
Copy link
Contributor

@z8v z8v commented Feb 18, 2023

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

@z8v z8v force-pushed the attr-path branch 2 times, most recently from 01a523c to f5b838b Compare February 18, 2023 23:14
@z8v z8v marked this pull request as ready for review February 19, 2023 10:20
Copy link
Contributor

@evertedsphere evertedsphere left a comment

Choose a reason for hiding this comment

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

Looks good!

"--argstr",
"ccType",
"ccTypeExpression",
"--arg",
Copy link
Member

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.

Copy link
Member

@benradf benradf Feb 21, 2023

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 😞)

Copy link
Contributor Author

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.

Copy link
Member

@benradf benradf left a 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.

@z8v z8v added the merge-queue merge on green CI label Feb 22, 2023
@mergify mergify bot merged commit e2ce883 into master Feb 22, 2023
@mergify mergify bot deleted the attr-path branch February 22, 2023 09:21
@mergify mergify bot removed the merge-queue merge on green CI label Feb 22, 2023
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.

3 participants