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

Add cpp linking test #1957

Closed
wants to merge 8 commits into from
Closed

Conversation

aidenfoxivey
Copy link
Contributor

Accidentally deleted #1944 and as a result I've remade the PR. I'll make sure to clean up the commits prior to merging if there's any desire to change what I've written here.

I've applied the advice from earlier to move the C++ example to .github and added a workflow to basic.yml.

@aidenfoxivey
Copy link
Contributor Author

aidenfoxivey commented Oct 22, 2024

I think I'll need to run CI to test whether basic.yml runs with changes - but I assume that's only something that reviewers and other org members can do.

@Martyrshot
Copy link
Member

Hm. @dstebila It seems like I lost the ability to unblock ci.

@aidenfoxivey
Copy link
Contributor Author

@Martyrshot Just to be certain, this is the location you had in mind for the C++ file, right?

@Martyrshot
Copy link
Member

Yes that's what I meant! Once CI is able to be run, I will give this a review. Initial scim looks good though!

@dstebila
Copy link
Member

Hm. @dstebila It seems like I lost the ability to unblock ci.

You're still listed on the liboqs-committers team so I would think you have the ability to unblock. @ryjones, any ideas? Could it be cause Aiden's PR is in his own fork?

@SWilson4
Copy link
Member

For what it's worth, I'm also not seeing any pending CI jobs here, and I certainly have perms to unblock them. I wonder if it's because there's a merge conflict in the basic workflow file?

@aidenfoxivey
Copy link
Contributor Author

For what it's worth, I'm also not seeing any pending CI jobs here, and I certainly have perms to unblock them. I wonder if it's because there's a merge conflict in the basic workflow file?

Maybe I can amend the commit to avoid having the merge conflict.

@aidenfoxivey aidenfoxivey force-pushed the main branch 3 times, most recently from bd95810 to 85d580c Compare October 24, 2024 15:23
@aidenfoxivey
Copy link
Contributor Author

Alright, we have a rather predictable issue with the path lol. I'll sort that out.

@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

Screenshot 2024-10-24 at 11 24 51 I clicked this button - did nobody else see it?

@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

So yes, it didn't run because it is on a fork, so it needs approval

@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

I might rebase your commit on top of main instead of having a merge commit

@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

I tried a rebase but there is a merge conflict

@aidenfoxivey
Copy link
Contributor Author

I tried a rebase but there is a merge conflict

Yeah, I had the same exact experience unfortunately. My git knowledge is not that good if I'm being honest.

@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

OK I re-wrote the commit let's see how it goes. I suspect the test cpp file needs to live elsewhere

Signed-off-by: Aiden Fox Ivey <[email protected]>
Signed-off-by: Ry Jones <[email protected]>
@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

OK @aidenfoxivey I think you can drive from here, I'll stop messing about

@aidenfoxivey
Copy link
Contributor Author

OK @aidenfoxivey I think you can drive from here, I'll stop messing about

Sounds good!

@aidenfoxivey
Copy link
Contributor Author

I suspect the issue is the prepended /.

Signed-off-by: Aiden Fox Ivey <[email protected]>
@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

@aidenfoxivey I restored the empty file

Signed-off-by: Ry Jones <[email protected]>
@aidenfoxivey
Copy link
Contributor Author

@aidenfoxivey I restored the empty file

awesome! let's see how this run goes

@ryjones
Copy link
Contributor

ryjones commented Oct 24, 2024

@aidenfoxivey are you able to build this locally?

Signed-off-by: Aiden Fox Ivey <[email protected]>
@aidenfoxivey
Copy link
Contributor Author

@aidenfoxivey are you able to build this locally?

yeah, locally it seems fine - that's what's confusing me.

@aidenfoxivey
Copy link
Contributor Author

CleanShot 2024-10-25 at 12 47 48@2x

I'm honestly a little bit confused. From what I can tell, this is the current name of the c++ file. Perhaps I'm misunderstanding what the current directory is in Github actions?

@ryjones
Copy link
Contributor

ryjones commented Oct 25, 2024

maybe do an ls -lR .* at the top?

Signed-off-by: Aiden Fox Ivey <[email protected]>
@aidenfoxivey
Copy link
Contributor Author

let's give this a go

Signed-off-by: Aiden Fox Ivey <[email protected]>
@aidenfoxivey
Copy link
Contributor Author

Whoops!

Signed-off-by: Aiden Fox Ivey <[email protected]>
@aidenfoxivey
Copy link
Contributor Author

Okay, sorted it out.

@aidenfoxivey
Copy link
Contributor Author

Hmm, so, if I understand correctly this is running from build, but somehow the header file isn't being found.

Signed-off-by: Aiden Fox Ivey <[email protected]>
@ryjones
Copy link
Contributor

ryjones commented Oct 25, 2024

@aidenfoxivey let me know when it is safe to rewrite your commit tree to make it shorter :)

@ryjones
Copy link
Contributor

ryjones commented Oct 25, 2024

Please see #1963

@ryjones ryjones closed this Oct 25, 2024
@ryjones
Copy link
Contributor

ryjones commented Oct 25, 2024

@baentsch baentsch mentioned this pull request Oct 30, 2024
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.

5 participants