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.

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

implement patch manually without using external software #136

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
3 changes: 3 additions & 0 deletions sys/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@ repository = "https://github.com/DelSkayn/rquickjs.git"

[build-dependencies]
cc = "1"
diffy = "0.3.0"
newline-converter = "0.3.0"
Comment on lines +15 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid patch number in dependency version.

path-absolutize = "3.1.1"

[build-dependencies.bindgen-rs]
package = "bindgen"
Expand Down
100 changes: 84 additions & 16 deletions sys/build.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
use std::{
env, fs,
io::Write,
path::Path,
process::{Command, Stdio},
path::{Path, PathBuf},
};

use diffy::{apply, Patch};
use newline_converter::dos2unix;
use path_absolutize::Absolutize;

fn main() {
#[cfg(feature = "logging")]
pretty_env_logger::init();
Expand Down Expand Up @@ -131,21 +133,87 @@ fn feature_to_define(name: impl AsRef<str>) -> String {
}

fn patch<D: AsRef<Path>, P: AsRef<Path>>(out_dir: D, patch: P) {
let mut child = Command::new("patch")
.args(["-p1"])
.stdin(Stdio::piped())
.current_dir(out_dir)
.spawn()
.expect("Unable to execute patch, you may need to install it: {}");
println!("Applying patch {}", patch.as_ref().display());
{
let patch = fs::read(patch).expect("Unable to read patch");

let stdin = child.stdin.as_mut().unwrap();
stdin.write_all(&patch).expect("Unable to apply patch");
struct Patches<'a>(&'a str);

// A backtracking attempt to find a valid diff when there are multiple patches in one file (also known as patchset)
impl<'a> Iterator for Patches<'a> {
type Item = Patch<'a, str>;
fn next(&mut self) -> Option<Self::Item> {
let mut range = self.0.len();

loop {
let input = if let Some(input) = self.0.get(..range) {
input
} else {
range -= 1;
continue;
};
match Patch::from_str(input) {
Ok(x) if x.hunks().is_empty() => break None,
Err(_) if range < 1 => break None,
Ok(x) => {
self.0 = &self.0[range..];
break Some(x);
}
Err(_) => range -= 1,
}
}
}
}

child.wait_with_output().expect("Unable to apply patch");
println!("Appliyng patch {}", patch.as_ref().display());
{
let out_dir = out_dir.as_ref();
let patch = fs::read_to_string(patch).expect("Unable to read patch");
let patch = dos2unix(&patch);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does we actually need that for both patch and source?
Does we need it at all?
I mean we may simply fix our patches if you have problem with it.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you determine which file to patch?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevefan1999-personal Course, I about line-ending conversion.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevefan1999-personal Course, I about line-ending conversion.

I'm not sure what you mean

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we need the following:

let patch = dos2unix(&patch);

and

let original = dos2unix(&original);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have CRLF in Windows and diffy does not support CRLF yet. There is no allocation cost if you have LF.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this could turn political quickly, but since QuickJS is written with GCC in mind, and is written by Fabrice Bellard, a legendary open source developer, I'm pretty sure he wrote this while only running on Linux, and Linux is LF by default. 

Yet on Git and Windows, you have automatic LR to CRLF conversion. It is up to the user to turn it off, but it seems like it is a viral option, and I believe this would cause even more unwanted trouble to end-users. 

I'm not aware if autocrlf can be turned off per Git repository or can be disabled for Git submodules but anyway, it is almost cost-free if you run this on Unix-like operating system. We should use do this conversion to "normalize" anyway.

Copy link

@rivy rivy Aug 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stevefan1999-personal

Using this .gitattributes will, by default, force text files to use LF EOLs, with CRLF exceptions where required for WinOS or MSVC.

# .gitattributes (git attributes config file)

# default; use LF EOLs for text files
* text=auto eol=lf

# CRLF required; force required CRLF EOLs for WinOS BAT/CMD and MSVC SLN/VCPROJ/VCXPROJ files
*.[Bb][Aa][Tt]                      text eol=crlf
*.[Cc][Mm][Dd]                      text eol=crlf
*.[Ss][Ll][Nn]                      text eol=crlf
*.[Vv][Cc][Pp][Rr][Oo][Jj]          text eol=crlf
*.[Vv][Cc][Pp][Rr][Oo][Jj].*        text eol=crlf
*.[Vv][Cc][Xx][Pp][Rr][Oo][Jj]      text eol=crlf
*.[Vv][Cc][Xx][Pp][Rr][Oo][Jj].*    text eol=crlf

for patch in Patches(&patch) {
let original = patch
.original()
.and_then(|x| {
if x.chars().next().unwrap_or_default() == '/' {
Some(x)
} else {
x.split_once('/').map(|(_, b)| b)
}
})
.or(patch.original())
.expect("Cannot find original file name");
let modified = patch
.modified()
.and_then(|x| x.split_once('/').map(|(_, b)| b))
.or(patch.modified())
.expect("Cannot find modified file name");

let original_path = if original.chars().next().unwrap_or_default() == '/' {
PathBuf::new()
} else {
out_dir.to_path_buf()
}
.join(original)
.absolutize()
.unwrap()
.to_path_buf();
let modified_path = out_dir.join(modified).absolutize().unwrap().to_path_buf();

let original = if !original_path.exists() {
String::new()
} else {
fs::read_to_string(original_path).expect("Unable to read original file")
};
let original = dos2unix(&original);
match apply(&original, &patch) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it support fuzzy patch applying?
Could you test build with parallel feature on windows?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about fuzzy patching, but I am building it with parallel and futures

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact is that MSVC patches changes line numbers and may interfere applying other patches.

Ok(patched) => {
if let Some(parent) = modified_path.parent() {
if !parent.exists() {
fs::create_dir_all(parent).unwrap();
}
}
fs::write(modified_path, patched).expect("Unable to write the patched content")
}
Err(e) => eprintln!("Unable to write the patched content: {}", e),
}
}
}
}

#[cfg(not(feature = "bindgen"))]
Expand Down