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

Add Windows CI support #161

Merged
merged 1 commit into from
Aug 20, 2021
Merged

Add Windows CI support #161

merged 1 commit into from
Aug 20, 2021

Conversation

chrissimpkins
Copy link
Collaborator

#157 includes platform-specific approaches to XML serialization. This PR adds a Windows test runner to the existing GitHub Actions CI configuration

@chrissimpkins
Copy link
Collaborator Author

Win tests are failing. It looks like the line endings may differ in features and glif XML vs. Linux env test expected values?


features: Some("@myClass = [A B];\r\n\nfeature liga {\r\n    sub A A by b;\r\n} liga;\r\n")
features: Some("@myClass = [A B];\r\n\r\nfeature liga {\r\n    sub A A by b;\r\n} liga;\r\n")

"@myClass = [A B];\r\n\nfeature liga {\r\n    sub A A by b;\r\n} liga;\r\n"
"@myClass = [A B];\n\nfeature liga {\n    sub A A by b;\n} liga;\n"

<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n
<?xml version=\"1.0\" encoding=\"UTF-8\"?>\r\n

@madig
Copy link
Collaborator

madig commented Aug 13, 2021

Funny, I'm not getting an error running it on my Windows workstation.

So, in glyph::tests::serialize_full_glyph, left is the roundtripped file with LF, right is the original with CRLF. Huh. Do we have a forced switch to LF?

@chrissimpkins
Copy link
Collaborator Author

chrissimpkins commented Aug 14, 2021

I think that the intent is to write LF on all platforms so that there is no platform-specific serialization diff?

@madig
Copy link
Collaborator

madig commented Aug 15, 2021

Hm, then these diffs shouldn't be happening? Will try to look into this next week.

@chrissimpkins
Copy link
Collaborator Author

chrissimpkins commented Aug 16, 2021

@cmyr Thoughts on how to approach cross-platform line endings? Should norad write consistent line endings on all platforms (limits diff when norad serialization is used x-platform)? Keep line ending format that it reads in on subsequent serialization (maybe limits diff when norad serialization is used with other serializers)? Allow custom line ending support (flexible and maybe limits diffs across serializers and platforms)?

@belluzj mentioned today that he likes the idea of a runebender config panel with Unix vs. Win line ending style options on UFO serialization. I think that we can expand the approaches used for spacing and XML declaration quote customization to address it if this is desirable. All of these options might be useful in such a config panel.

@cmyr
Copy link
Member

cmyr commented Aug 16, 2021

I'd be most interested in knowing if there is an accepted convention for this. In the general case I would expect a tool to use the correct line endings for the given platform, unless there is an established convention to do otherwise?

@chrissimpkins
Copy link
Collaborator Author

@cmyr Thoughts on how to approach cross-platform line endings? Should norad write consistent line endings on all platforms (limits diff when norad serialization is used x-platform)? Keep line ending format that it reads in on subsequent serialization (maybe limits diff when norad serialization is used with other serializers)? Allow custom line ending support (flexible and maybe limits diffs across serializers and platforms)?

@belluzj mentioned today that he likes the idea of a runebender config panel with Unix vs. Win line ending style options on UFO serialization. I think that we can expand the approaches used for spacing and XML declaration quote customization to address it if this is desirable. All of these options might be useful in such a config panel.

Made this an issue #162

@madig
Copy link
Collaborator

madig commented Aug 18, 2021

Ok, so, I see this:

  1. The test font::tests::upconversion_fontinfo_v123 fails because I insert a literal \n when upconverting v1 font info but the reference feature.fea compared against is using the platform \r\n line endings. For some reason, I see the files with LF on my Linux PC. So, I'm not sure if Git on the Windows CI converts stuff to CRLF?
  2. The test font::tests::loading fails because the feature file read from disk is using CRLF but we are using LF in the comparison string
  3. The test font::tests::upconvert_ufov1_robofab_data fails because my literal \n insertion results in a mix of CRLF (read from disk) and LF while the comparison string uses LF
  4. The test glyph::tests::serialize_full_glyph fails because LF is read from disk (via include_bytes!), CRLF is written to the comparison buffer.

I'm not sure why some things are read with LF and others with CRLF, but I suppose the default should be to read and write (and insert) stuff using the platform defaults. Then, optionally, one can force all output to use LF.

@cmyr
Copy link
Member

cmyr commented Aug 19, 2021

Okay, some notes:

  • writeln! and friends do not change behaviour based on platform; it's always \n.
  • as far as I can tell, the issue is with git's handling of line endings; basically git is generally configured to convert line endings on checkout.

I think that a reasonable solution to this is using a .gitattributes file to ensure that the line endings in test data is preserved on checkout... it's possible I'm overlooking some reason that this is a bad idea, but I can't think of anything.

Okay, actually, there are two options: we can either ask git to treat things like .fea and .glif files as 'binary' files, in which case line endings aren't touched, or we can ask git to always use LF, even on linux. I think maybe I prefer the binary approach? I'll play around, but input is welcome

@cmyr
Copy link
Member

cmyr commented Aug 19, 2021

Okay, this last failure is because MutatorSans is a submodule, and is not in the actual tree. I think we should just take one of the MutatorSans fonts and add it to the tree?

@madig
Copy link
Collaborator

madig commented Aug 19, 2021

Hm, we can, yes, but I'm still confused by the CRLF/LF behavior. Python converts everything to \n internally for ease of use but what does Rust do where? Should we take care of CRLF/LF when manipulation strings in norad?

@cmyr
Copy link
Member

cmyr commented Aug 19, 2021

In most cases, we do not directly manipulate text files; we generally parse them into objects, and the parser discards the newlines.

The main exception is FEA files, which we read directly. When reading a file, Rust reads it literally, and the line endings in the loaded file's contents will be the line endings on disk.

So the main question is, "when writing files back out to disk, what line endings should norad use"? And I don't have strong opinions. In particular, we would need to figure out the behaviour of the plist and quick_xml crates, since they handle the actual writing for us.

The main question for me is: what behaviour do we want? Are there any conventions here? Once we know what we want, we can see what's involved in making it happen.

@chrissimpkins
Copy link
Collaborator Author

chrissimpkins commented Aug 19, 2021

Okay, this last failure is because MutatorSans is a submodule, and is not in the actual tree. I think we should just take one of the MutatorSans fonts and add it to the tree?

Separate issue in support of this. I noticed over the weekend that this approach recursively pulls the submodule in ufofmt builds that use a git repo revision def in the Cargo.toml. I don't know if moving away from a git submodule addresses that?

@cmyr
Copy link
Member

cmyr commented Aug 19, 2021

Separate issue (possibly) in support of this. I noticed over the weekend that this approach recursively pulls the submodule in ufofmt builds that use a git repo revision def in the Cargo.toml. I don't know if moving away from a git submodule addresses that?

It should!

@madig
Copy link
Collaborator

madig commented Aug 19, 2021

I think we should write platform line endings by default but have a writer option for forcing LF.

@cmyr
Copy link
Member

cmyr commented Aug 20, 2021

@chrissimpkins I'm going to rebase and force push this, apologies if I'm toe-stepping

@cmyr cmyr merged commit dc1ecb9 into linebender:master Aug 20, 2021
@chrissimpkins chrissimpkins deleted the windows-ci branch August 20, 2021 15:45
@chrissimpkins
Copy link
Collaborator Author

@chrissimpkins I'm going to rebase and force push this, apologies if I'm toe-stepping

Not at all.

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.

None yet

3 participants