-
-
Notifications
You must be signed in to change notification settings - Fork 144
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
Switch to CircleCi #233
Switch to CircleCi #233
Conversation
dman.d
Outdated
@@ -142,7 +142,7 @@ string CHeader(string topic) | |||
static string[] dmccmds = | |||
[ | |||
"assert.h", "complex.h", "ctype.h", "fenv.h", | |||
"float.h", "locale.h", "math.h", "setjmp.h," | |||
"float.h", "locale.h", "math.h", "setjmp.h,", |
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.
Good catch but I think the comma should also be removed from the string literal.
da75a32
to
10f5376
Compare
{ | ||
changes = true; | ||
de._madeVersion = 0x317; // necessary or linux unzip will ignore attributes | ||
de.externalAttributes = (de.externalAttributes & ~(octal!7777 << 16)) | (newattr << 16); | ||
de.fileAttributes = de.fileAttributes & ~octal!7777 | newattr; |
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'm not sure about these changes - on one hand if the script wasn't building no idea if it was actually used any more, on the other hand this needs to work correctly so that unzipping .zip files on POSIX results in the correct attributes being set for files, and I doubt this is tested automatically anywhere right now. @MartinNowak ?
@@ -54,6 +54,13 @@ void main(string[] args) | |||
"concurrency", &concurrencyTest, | |||
); | |||
|
|||
// if the compiler contains a dir separator, it's not the in the global PATH |
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 there is some grammar bug here
// an absolute path or executable in the PATH is required as the test suite | ||
// used chdir | ||
if (compiler.canFind(dirSeparator)) | ||
compiler = compiler.asAbsolutePath.array; |
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.
Why not just .toAbsolutePath
?
// for absolute compiler path, we make the path relative again | ||
string fullCompilerPath = void; | ||
if (compiler.isAbsolute) | ||
fullCompilerPath = compiler.asRelativePath(getcwd()).array; |
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.
ditto
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.
Wait, why is this branch computing a relative path when the other branch is computing an absolute one?
.front; | ||
// for absolute compiler path, we make the path relative again | ||
string fullCompilerPath = void; | ||
if (compiler.isAbsolute) |
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 guess technically this wasn't necessary since buildPath
with an absolute path will always result in that absolute path, but I guess this is more efficient and results in a better error if the path doesn't exist.
build-tools) build_tools;; | ||
test-rdmd) test_rdmd ;; | ||
*) echo "Unknown command"; exit 1;; | ||
esac |
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.
Lots of boilerplate that's going to need to be kept in sync across repos. Any ideas on DRY?
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.
Also, how feasible is to have a single script file that works with both Travis and Circle CI?
dman.d
Outdated
@@ -142,7 +142,7 @@ string CHeader(string topic) | |||
static string[] dmccmds = | |||
[ | |||
"assert.h", "complex.h", "ctype.h", "fenv.h", | |||
"float.h", "locale.h", "math.h", "setjmp.h,", | |||
"float.h", "locale.h", "math.h", "setjmp.h", |
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.
Should be fixup'd, the other commits are good separately.
Reboot of #205 to use CircleCi instead of Travis as Travis is to clogged from DMD. For now this tests only 64-bit.
It worked on my local fork: https://circleci.com/gh/wilzbach/tools/4