Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Reenable Windows CI build with Artifactory support #4596
Reenable Windows CI build with Artifactory support #4596
Changes from 3 commits
ab52f3c
3df07fb
f31ebff
e0a84e0
10f25ff
e88ca37
1b5e089
9c0882f
069aebb
ab26e5c
106596b
b42edc1
5cfa42c
5d661f5
f568ac0
f01002f
9c15e2a
77a8f2c
de2df5d
a35dd82
efd32a6
0fecaf7
4cd85a6
43690b3
2f3ddde
222c7e4
715aeea
1641cc6
f875a86
63c6717
e308801
8f1c1e4
15f1f0c
5778a34
7fdd63c
c3bce68
5f3a938
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Was this actually necessary? I don't think the build directory has multiple steps, so I'm guessing this was added to get around an existing build directory. I suspect we should instead remove and remake the build directory if that was the reason.
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 was to work around an existing directory. IIRC, the workflow has a "build dependencies" step, which calls this script directly and a "build" step, which calls this workflow indirectly. It must have been making the second step fail.
I'll go ahead and remove the change, and see how that affects things.
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.
@ximinez are you working with your local windows machine or tinkering with the Windows runner provided by Github Actions?
I don't have a windows machine and I'm wondering how I can try out these build steps.
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.
Yep, that was it. https://github.com/ximinez/rippled/actions/runs/6343277082/job/17230820510#step:13:61
The reason it's not an issue on the
nix
jobs is that those two steps are split across the separate jobs. My problem is a side effect of bringing them back together. I can think of three ways to make this work.-p
back with a comment explaining that it's for this scenario.rm -rf ${build_dir}
before creating it to ensure we're starting with a fresh slate.[ -d ${build_dir} ] || mkdir ${build_dir}
so it doesn't error out if it exists.I don't really have a strong feeling one way or another. 2 is probably the safest option, even though it'll add a little time to the build because it'll need to recreate the conan toolchain file. What do you think?
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.
This is with the Windows runner from Github Actions.
You could copy my branch into a branch in your own repo and play with it from there.
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.
okay, I'll check it out 👍
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 the right fix is to remove the link between the
build
anddependencies
actions, and just add a step for thedependencies
action to thetest
job in thenix
workflow.Alternatively, but not my preference, you could remove the
dependencies
step in thewindows
workflow and move the upload step after thebuild
step, adding analways() &&
clause to itsif
condition.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.
Can we drive-by add the
--settings build_type=${{ matrix.configuration }}
fix in thenix
workflow too?