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.

Sign up for GitHub

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

Jump to bottom

test_bad_locking: make merge_directories() expect non-existent target #2262

Merged
merged 1 commit into from
Sep 18, 2023
Merged

test_bad_locking: make merge_directories() expect non-existent target #2262

merged 1 commit into from
Sep 18, 2023

Conversation

martinvonz
Copy link
Member

@colemickens, let me know if this makes any difference to #2103.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@colemickens
Copy link
Contributor

colemickens commented Sep 17, 2023 •
edited
Loading

Might be hard to say, I just re-added jj to my config and it seemed to just build. That is, the latest version from this repo, not this PR.

@martinvonz
Copy link
Member Author

Might be hard to say, I just re-added jj to my config and it seemed to just build. That is, the latest version from this repo, not this PR.

Weird that it's different now. I wish we knew why, but at least it's working for you (for now anyway), and I think this PR does fix some issues.

@colemickens
Copy link
Contributor

Depending on how curious you are, I can rebuild with --rebuild in a loop and see if it fails.

Alternatively, now that I've re-enabled it, I'll end up rebuilding this somewhat often and will let you know if I see failures again (before or after you merge this).

@martinvonz
test_bad_locking: make merge_directories() expect non-existent target
9c475c5 
I ran into some issues here when switching our tests to use
`.jj`-internal git repos. For example, the `std::fs::copy()` calls
started failing, which may be related to #2103. I think one problem is
that we could end up calling `merge_directories()` twice for the same
directory. This patch fixes that by deduping the paths we call with,
and makes the function assume that the output directory doesn't exist.
@colemickens
Copy link
Contributor

Well, okay, with tip of tree, this early afternoon:

  • x86_64-linux (on a quad-core, ht system): build failed on these tests
  • aarch64-linux (on a huge Ampere system): passed

When I switched my jj input to this PR's branch, it built on both systems first try.

Sample size of 1 each, basically, but it's what I can offer for now.

@martinvonz martinvonz merged commit 2bc641c into main Sep 18, 2023
15 checks passed
@martinvonz martinvonz deleted the push-untysktpqvzy branch September 18, 2023 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuja yuja yuja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@martinvonz @colemickens @yuja