-
Notifications
You must be signed in to change notification settings - Fork 952
Improve build time of x86_64-linux-gnu target #1760
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
7f3b93f
to
b9a4f4c
Compare
r? @kinnison |
769deab
to
9f55e76
Compare
RUN ./configure.gnu | ||
RUN make -j$(nproc) | ||
RUN make install | ||
RUN curl -L https://github.com/lzutao/rustup.rs/releases/download/perl-5.28.0/perl-5.28.0.tar.gz | tar xzf - -C / |
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.
We should upload it to somewhere rather than in my own fork.
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.
Certainly I wouldn't like this part of the change. I do feel like we can find a better solution where we don't use non-packaged perl at all, but I don't see what it is yet.
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.
It's better but not the best solution we have. Should we ping alexcrichton
to see what he think about this?
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.
Yeah... @alexcrichton If you have a moment could you weigh in on a better way to do this part of rustup
CI? I really don't like storing built binaries of Perl like this, but I also dislike that we end up rebuilding perl an awful lot.
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.
Instead of a prebuilt perl, could this perhaps take the same strategy as #1724 and use the prebuilt images from rust-lang/rust for deployment?
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 did the change in my branch.
The Travis build log here.
There are two blocking points:
- The build is failed, but I don't see why it's failed. It complains about
linker `cc` not found
, but gcc and g++ is included in rust
x86_64 and i686 docker image? - The build time is increase. The average build time is 17 minutes and 30
seconds (with build succeed and all tests passed).
Now the build time is at least 18 minutes (with tests uncompleted).
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.
It's possible that travis will cache the image now for us, reducing the cost of using a larger docker image, but the cc
not found is a pain.
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.
@kinnison We use rust's images. Those images could be changed or updated so the
cache will be invalid (correct me if I'm wrong).
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.
It would, but not very often. (I think at least every 90 days but likely more often than that) so I think it won't be too bad.
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 forgot. We download the tar-compressed docker images from aws with curl (about 1GB). So I don't think Travis would (automatically) cache this file for us. We could force Travis cache this file, but it's not recommended by Travis and is inefficient.
@lzutao Is it possible to use POSIX-compatible shell instead of bash? The CI script named |
37a4d53
to
501533a
Compare
@pickfire All shell scripts should be POSIX-compliant now, except the |
@lzutao Is it possible to remove to |
@pickfire I don't think that removing |
adc56b5
to
863aca5
Compare
@lzutao I understand your concern but different parts of scripts are run separately and the use of |
IIRC, dash supports |
0984730
to
eb4547c
Compare
Wait, I tested it on function level and it works but not on global level. |
I think currently we only use |
This would reduce build time upto 3 minutes. The final Perl tarball was built in the same image from Dockerfile.
Closed in favor of #1815 . |
By using prebuilt perl, we could reduce build-time up to 3 minutes.