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

Conversation

stevefan1999-personal
Copy link

This PR attempts to manually do patches without using GNU Patch. Despite the algorithm is quite primitive and had some questionable assumptions, at least it does compile in Windows natively now. The edge case is that I don't know how to determine the correct file to patch -- so I just assumed the a/{filename} and b/{filename} format.

Fixes #88. Since I made this in Windows and I have to compile it by myself. So I can't possibly made this PR possible if there are any errors. No need to test for Windows therefore.

let original =
fs::read_to_string(out_dir.join(original)).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.

Comment on lines +15 to +16
diffy = "0.3.0"
newline-converter = "0.3.0"
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.

sys/build.rs Outdated
.expect("Cannot find modified file name");

// for now
assert_eq!(original, modified);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove that.

stdin.write_all(&patch).expect("Unable to apply patch");
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to build on windows
3 participants