-
Notifications
You must be signed in to change notification settings - Fork 177
prepare project for public CI #2
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
Conversation
motivation: get ready to open source project changes: * add sanity script * add doc generation script * add docker and docker-compose setup * run swiftformat * remove generate_linux_tests.rb * add template for github PRs * add tempalte for git commit message * add api-breakage detection scipt
test: | ||
<<: *common | ||
# FIXME: get rid of existing warnings | ||
#command: /bin/bash -xcl "swift test --enable-test-discovery -Xswiftc -warnings-as-errors $${SANITIZER_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.
@Lukasa note the FIXME above
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.
Yup, I'm aware of these warnings: they're tracked by a bugs.swift.org ticket.
api-breakage expected to fail until the repo is public, will disable it for now |
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 this @tomerd! A few small requests, if you don't mind.
test: | ||
<<: *common | ||
# FIXME: get rid of existing warnings | ||
#command: /bin/bash -xcl "swift test --enable-test-discovery -Xswiftc -warnings-as-errors $${SANITIZER_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.
Yup, I'm aware of these warnings: they're tracked by a bugs.swift.org ticket.
@Lukasa wdyt |
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.
LGTM, thanks Tom!
…apple#46) ### Motivation When running the cancellation tests in a loop, very occasionally there would be a crash with the following backtrace: ``` #0 0x000000018aa008d4 in objc_opt_respondsToSelector () apple#1 0x000000018aea3410 in _outputStreamCallbackFunc () apple#2 0x000000018aea3310 in _signalEventSync () apple#3 0x000000018aeecdb0 in ___signalEventQueue_block_invoke () apple#4 0x000000018abe6cb8 in _dispatch_call_block_and_release () apple#5 0x000000018abe8910 in _dispatch_client_callout () apple#6 0x000000018abefea4 in _dispatch_lane_serial_drain () apple#7 0x000000018abf0a08 in _dispatch_lane_invoke () apple#8 0x000000018abfb61c in _dispatch_root_queue_drain_deferred_wlh () apple#9 0x000000018abfae90 in _dispatch_workloop_worker_thread () apple#10 0x000000018ad96114 in _pthread_wqthread () ``` This seems to indicate that the output stream is trying to access its delegate. However, when running with debug logging enabled I can see that the delegate has already been deinitialized. This is likely a result of the delegate itself owning the stream and setting the stream delegate to `self`, which IIUC is an established pattern. This presents a race in teardown. ### Modifications This patch sets the output stream delegate to `nil` in the delegate `deinit`. ### Result No attempts to call the delegate will happen after it is has been deinitailzed. ### Test Plan With this patch, the failing test passes when run an order of magnitude more times than were required to reliably reproduce the crash without the patch.
motivation: get ready to open source project
changes: