-
Notifications
You must be signed in to change notification settings - Fork 4
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
Fix build system #9
Conversation
build_system/src/build.rs
Outdated
@@ -29,6 +29,16 @@ impl BuildArg { | |||
); | |||
} | |||
} | |||
"--sysroot" => { | |||
if let Some(arg) = args.next() { | |||
build_arg.flags.push("--sysroot".to_string()); |
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 not what should be done.
Instead, this new --sysroot
flag that you added should be used to determine if we should build the sysroot or not. This happens here.
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 we should call build_sysroot(&env, &args.config_info)?;
on the if check
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.
In this function, set a new field that you can add on this struct to true
and in the link from my comment above, wrap the call to build_sysroot
in an if
that will check if this new field is set to true
.
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.
adding a boolean flag on this BuildArg
, then set it to TRUE on --sysroot
passed
build_system/src/test.rs
Outdated
@@ -138,6 +138,16 @@ impl TestArg { | |||
return Err("Expected an argument after `--features`, found nothing".into()) | |||
} | |||
}, | |||
"--sysroot" => match args.next() { |
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 file already contain an option to build the sysroot here, so it should not be needed.
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 will remove 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.
The CI failed, so the code needs a fix, but looks good otherwise.
build_system/src/build.rs
Outdated
@@ -9,6 +9,7 @@ use std::path::Path; | |||
struct BuildArg { | |||
flags: Vec<String>, | |||
config_info: ConfigInfo, | |||
sys_root: bool, |
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 would prefer this spelling:
sys_root: bool, | |
sysroot: bool, |
what could make the CI failed |
You can see the results of the CI at the bottom of this page. If you click on this one, for example, it shows you the build errors. |
There's still one error (and a warning) to fix in the CI. Keep up the good work. Btw, do you get notified for the CI failures? |
Yes I get notifications for CI failure , |
the error is from |
The new error in the CI is because you now need to use this new |
you mean something like this |
No, the argument you added is for the |
Alright |
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 seems the sysroot is still not built in the CI.
Does this work locally on your side?
build_system/src/build.rs
Outdated
@@ -220,8 +225,11 @@ fn build_codegen(args: &mut BuildArg) -> Result<(), String> { | |||
})?; | |||
|
|||
println!("[BUILD] sysroot"); |
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.
Please move this line inside the if
.
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.
Alright, could be the reason why the CI is failing
am getting the following on local
|
@@ -224,7 +224,6 @@ fn build_codegen(args: &mut BuildArg) -> Result<(), String> { | |||
) | |||
})?; | |||
|
|||
println!("[BUILD] sysroot"); | |||
if args.sysroot { |
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 mean to actually move it like this:
println!("[BUILD] sysroot");
if args.sysroot {
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 see
You're missing the actual build of the sysroot.
Do you know why it doesn't work with the code from this PR? |
i have no idea, i will switch to the master and run the build |
Looking at logs again, i keep seeing error below, as found here
|
Yes, this is because the sysroot is not actually built. |
I will look at the places where I made the changes on my branch and compare it the place on the master branch |
after looking at where i made change on my PR Branch and the master branch, this is piece of code that i added |
gccjit_target, error | ||
) | ||
})?; | ||
|
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.
Perhas you could try print the field to see if we actually reach this line before the if
:
println!("Build sysroot: {}", args.sysroot);
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.
Yaah, i will try 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.
It is not even printed, so we don't even reach this code.
Do you have any idea what in this PR could have the effect of skipping this code?
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 suspect the build command
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.
You can try adding println!
calls in a few places (especially where you changed the code) to see where it stops happening.
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.
so far, this is the only place, we added the sysroot flag
- name: Build
run: |
./y.sh prepare --only-libcore
./y.sh build --sysroot
cargo test
in the ci.yml file
i remove it from all other places i added it
i will add |
where could this error come from |
This is because the sysroot is not built. Building the sysroot means building |
should i pull out a new branch from master and push my work on it, i feel like the current branch on this PR is currupt |
Yes, you can do that. |
Was superceeded by #13. |
No description provided.