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

Set up publishing of @webref/idl with Lerna #68

Merged
merged 8 commits into from
Feb 12, 2021
Merged

Conversation

foolip
Copy link
Member

@foolip foolip commented Oct 26, 2020

Lerna is a tool for managing monorepos, and while @webref/idl is the
only package here now, the packages/* structure makes sense.

Fixes #63.

@foolip foolip force-pushed the webref-idl-package branch 2 times, most recently from 8a39b0c to dc87a1a Compare October 26, 2020 18:37
@foolip
Copy link
Member Author

foolip commented Oct 26, 2020

@dontcallmedom does this look sensible?

@saschanaz
Copy link
Member

Can we also publish CSS things? I can try it as a separate PR though.

@foolip
Copy link
Member Author

foolip commented Nov 10, 2020

Ping @dontcallmedom for a sensibility review.

What's missing here is still the mechanism by which the IDL files will be fixed to be consistent. Options I've considered:

  • A branch with changes which we keep rebasing, so we always release this package from that branch.
  • A set of patches (as files) which are applied after copying ed/idl/* into packages/idl/.
  • A branch which we treat as a big patch and apply after copying ed/idl/* into packages/idl/.

The last option is perhaps my favorite one, but it's a bit iffy that the repo wouldn't work as a shallow checkout, but depends on its own history.

@dontcallmedom
Copy link
Member

my own initial idea was to maintain the patches as files in the main branch, so that the content of the package be self-contained. What are the advantages of using a dedicated branch?

Whichever way is picked, failing visibly when the patch no longer applies will be important.

@saschanaz
Copy link
Member

The approach I'm using on TSJS is to simply ignore the failing upstreams and maintain my own IDL. Is there a reason to keep both failing and patched ones?

@foolip
Copy link
Member Author

foolip commented Feb 10, 2021

@dontcallmedom, @tidoust and I met and discussed this in a VC today. The main outstanding issue is how to maintain the fixes, as discussed in #68 (comment). Out of the options listed there, maintaining a bunch of patches as files in the repo is the easiest to get started with, so let's go with that.

@foolip foolip force-pushed the webref-idl-package branch 3 times, most recently from f106801 to 4f399f1 Compare February 10, 2021 13:11
Lerna is a tool for managing monorepos, and while @webref/idl is the
only package here now, the packages/* structure makes sense.

Fixes #63.
@foolip foolip marked this pull request as ready for review February 10, 2021 13:19
@foolip
Copy link
Member Author

foolip commented Feb 10, 2021

@dontcallmedom @tidoust this is ready for review now. I can't really test if the published package will look as hoped, but if we publish 1.0.0 we'll quickly know :)

@saschanaz only the fixed IDL will be published, if anyone enjoys broken IDL there's always ed/idl/*.idl in this repo :)

Co-authored-by: Dominique Hazael-Massieux <dom@w3.org>
test/idl/all.js Outdated Show resolved Hide resolved
@dontcallmedom
Copy link
Member

I've been able to publish the 1.0.0 of the package using this pull request: https://www.npmjs.com/package/@webref/idl 🎉

@tidoust will provide a JS rewrite of prepare.sh (so that it can be used on Windows)

@tidoust
Copy link
Member

tidoust commented Feb 11, 2021

@tidoust will provide a JS rewrite of prepare.sh (so that it can be used on Windows)

That's not needed, after all. The problem is that the default (and recommended) Git config on Windows makes it convert LF line endings to CRLF in text files, and bash does not like that. I'll add a .gitattributes file to tell Git to keep LF line endings for .sh files.

That said, with that fix, I note I still get the following error when I run the prepare.sh script for the SRI patch that should delete the file:

patching file SRI.idl
Reversed (or previously applied) patch detected!  Assume -R? [n]

Anyone knows what may explain this?

@foolip
Copy link
Member Author

foolip commented Feb 11, 2021

@tidoust SRI.idl should indeed be deleted, and using the version of patch on my system I don't get such a complaint. But I guess that's not universal. We could instead just rm it in prepare.sh.

@tidoust
Copy link
Member

tidoust commented Feb 11, 2021

Same cause, different consequences...

patch applies some heuristic to convert CRLF line endings to LF on Windows machines and so patch no longer matches. That problem is not restricted to the SRI patch, it just happens to be the first one to crash the script on my machine. If I remove the patch, other patches fail too with a more explicit "different line endings" error message.

The --binary option disables that behavior and solves the issue on my end (and should not have any impact on POSIX systems).

- The .gitattributes file tells Git not to convert LF line endings of the bash
script. The script cannot run on typical Git configs on Windows otherwise.
- The `--binary` option tells `patch` to disable any CRLF to LF conversion
when applying the patches. That is also needed for patches to run smoothly on
Windows.
@foolip
Copy link
Member Author

foolip commented Feb 11, 2021

It looks like we've found a situation where just a branch would have been easier to deal with, but oh well. Let's add in --binary and hope there's not a long list of issues like this waiting for us.

Since 1.0.0 was already published, it would be great to land this. I've discovered some issues when trying to integrate this into mdn-bcd-collector, but I'd like the original state from which 1.0.0 was published to be in the repo.

@tidoust
Copy link
Member

tidoust commented Feb 11, 2021

Since 1.0.0 was already published, it would be great to land this

Can we just drop the hardcoded number of IDL files (198) in the test before we merge? Or is there a rationale to have that number hardcoded?

@dontcallmedom
Copy link
Member

I think this is now ready to merge - @tidoust do you confirm?

Thanks a lot @foolip for driving this forward!

@foolip
Copy link
Member Author

foolip commented Feb 12, 2021

I fixed a few more small things. Please squash and keep only the first commit message, because I made no effort :)

@tidoust tidoust merged commit 5d1de8e into master Feb 12, 2021
@foolip foolip deleted the webref-idl-package branch February 12, 2021 08:15
@foolip foolip restored the webref-idl-package branch February 12, 2021 13:02
@foolip foolip deleted the webref-idl-package branch February 12, 2021 13:03
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.

Publish crawled data with fixes to NPM?
4 participants