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

mingw-w64-asciidoctor-extensions: add arm64 support #118

Closed
wants to merge 1 commit into from

Conversation

dennisameling
Copy link

Ruby support was added to clangarm64 in November 2023, so we can now enable asciidoctor-extensions on ARM64 as well. This is needed for building the mingw-w64-git package.

Ref: msys2#19179

Ruby support was added to clangarm64 in November 2023, so we can now enable asciidoctor-extensions on ARM64 as well. This is needed for building the mingw-w64-git package.

Ref: msys2#19179
Signed-off-by: Dennis Ameling <[email protected]>
pkgver=150.9846486
pkgver=246.42e366e
Copy link
Author

Choose a reason for hiding this comment

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

The previous commit that was used is from 2017. I find it a bit tricky to bump it so much. We can also pin it to that specific commit instead. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

My preference is always to lag behind as little as possible without breaking things.

@dennisameling
Copy link
Author

The CI run is failing because the copy-to-clipboard-docinfo-processor lib has an img folder in it:

install: omitting directory '/d/a/MINGW-packages/MINGW-packages/mingw-w64-asciidoctor-extensions/src/asciidoctor-extensions/lib/copy-to-clipboard-docinfo-processor/img'
==> ERROR: A failure occurred in package().
    Aborting...

When I remove that from the src directory and run makepkg-mingw -sLf --noextract, the build works fine.

So I think we have two options here:

  • Fix this package to the older 2017 commit 9846486 which I referenced above
  • Fix the prepare() logic so that it can deal with subfolders

WDYT?

@dennisameling dennisameling requested review from dscho and rimrul May 15, 2024 16:50
dennisameling added a commit that referenced this pull request May 15, 2024
…eded

Ruby support was added to clangarm64 in November 2023, so we can now enable asciidoctor-extensions on ARM64 as well.

Ref: msys2#19179
Ref: #118
Signed-off-by: Dennis Ameling <[email protected]>
pkgver=150.9846486
pkgver=246.42e366e
Copy link
Member

Choose a reason for hiding this comment

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

My preference is always to lag behind as little as possible without breaking things.

@dscho
Copy link
Member

dscho commented May 15, 2024

Fix the prepare() logic so that it can deal with subfolders

You mean the package() logic, right? I think it's probably this line:

    install -m755 "$srcdir/${_realname}/lib/$extension"/* "$pkgdir$LIBDIR/$extension"

Are the files in those extension directories typically .rb files? If so, then changing * to *.rb is easy enough, we just have to add something like:

  if test -d "$srcdir/${_realname}/lib/$extension"/img
  then
    install -d -m755 "$pkgdir$LIBDIR/$extension/img"
    install -m644 "$srcdir/${_realname}/lib/$extension"/img/* "$pkgdir$LIBDIR/$extension/img"
  fi

@dennisameling
Copy link
Author

You mean the package() logic, right?

Exactly. It's been a long day, sorry 🙃

Are the files in those extension directories typically .rb files?

Looks like it's a mix of .rb and .adoc files, so I could filter on those extensions indeed.

Do we need these extensions in the first place? The repo description worries me a bit:

A lab for testing and demonstrating Asciidoctor extensions. Please do not use this code in production. If you want to use one of these extensions in your application, create a new project, import the code, and distribute it as a RubyGem. You can then request to make it a top-level project under the Asciidoctor organization.

Shouldn't we use mingw-w64-asciidoctor in mingw-w64-git instead of having this custom package?

@dscho
Copy link
Member

dscho commented May 15, 2024

Looks like it's a mix of .rb and .adoc files, so I could filter on those extensions indeed.

Right, *.* would do it.

Do we need these extensions in the first place? The repo description worries me a bit:

A lab for testing and demonstrating Asciidoctor extensions. Please do not use this code in production. If you want to use one of these extensions in your application, create a new project, import the code, and distribute it as a RubyGem. You can then request to make it a top-level project under the Asciidoctor organization.

Shouldn't we use mingw-w64-asciidoctor in mingw-w64-git instead of having this custom package?

When I added that dependency, it was required by Git. I think it had something to do with linkgit. Let me see whether I can find it quickly.

@dscho
Copy link
Member

dscho commented May 15, 2024

When I added that dependency, it was required by Git. I think it had something to do with linkgit.

git/git@55d2d81 is the commit that introduced that dependency. It's still in effect, so I fear that we require the extensions.

@rimrul
Copy link
Member

rimrul commented May 16, 2024

When I added that dependency, it was required by Git. I think it had something to do with linkgit.

git/git@55d2d81 is the commit that introduced that dependency.

Did it, though? I think git/git@ec3366e introduced that dependency and git/git@55d2d81 removed it.

@rimrul
Copy link
Member

rimrul commented May 16, 2024

And you even started the discussion that lead to it's removal.

https://lore.kernel.org/git/alpine.DEB.2.20.1701251425080.3469@virtualbox/

@dennisameling
Copy link
Author

Ah interesting, so this seems to be only about man-inline-macro then. git/git@55d2d81 indeed made that available in the git repo directly, which likely means that we no longer need to install these extensions separately.

I think it might be worth trying mingw-w64-asciidoctor in mingw-w64-git and see if it works, WDYT?

@dscho
Copy link
Member

dscho commented May 16, 2024

And you even started the discussion that lead to it's removal.

https://lore.kernel.org/git/alpine.DEB.2.20.1701251425080.3469@virtualbox/

Wow, 🤣, blast from the past.

I think it might be worth trying mingw-w64-asciidoctor in mingw-w64-git and see if it works, WDYT?

Yes! I wonder, though, whether this will work, given that we still pass the -rasciidoctor-extensions option to asciidoctor. But then, that might no longer be necessary?

@rimrul
Copy link
Member

rimrul commented May 16, 2024

Yes! I wonder, though, whether this will work, given that we still pass the -rasciidoctor-extensions option to asciidoctor. But then, that might no longer be necessary?

I think that refers to the confusingly named file Documentation/asciidoctor-extensions.rb.

Looking at #6 it appears like Git for Windows might have used asciidoctor (and mingw-w64-asciidoctor-extensions) before there was support in git.git.

@dscho
Copy link
Member

dscho commented May 16, 2024

I think that refers to the confusingly named file Documentation/asciidoctor-extensions.rb.

Ah, https://github.com/git/git/blob/v2.45.1/Documentation/asciidoctor-extensions.rb is indeed what this includes, and that's indeed because of my wish. Thank you for digging into that @rimrul!

@dennisameling that means the road is clear for a mingw-w64-asciidoctor-extensions-less future! Do note, though, that this package had its challenges in the past and the corresponding set of work-arounds (also this). Meaning: There might be a long tail of the project to drop that dependency ;-)

dennisameling added a commit that referenced this pull request May 16, 2024
While enabling clangarm64 support on GfW's
`mingw-w64-asciidoctor-extensions` package, we realized that the
extensions are likely not needed anymore, because the Git project
added its own `Documentation/asciidoctor-extensions.rb` back in 2017.

Let's switch to the MSYS2-maintaned `mingw-w64-asciidoctor` instead,
so we don't have to maintain our own package anymore.

Ref: #118
Ref: git/git@55d2d81
Signed-off-by: Dennis Ameling <[email protected]>
dennisameling added a commit that referenced this pull request May 16, 2024
Ruby support was added to clangarm64 in November 2023,
so we can now enable asciidoctor on ARM64 as well.

Ref: msys2#19179
Ref: #118
dennisameling added a commit that referenced this pull request May 16, 2024
While enabling clangarm64 support on GfW's
`mingw-w64-asciidoctor-extensions` package, we realized that the
extensions are likely not needed anymore, because the Git project
added its own `Documentation/asciidoctor-extensions.rb` back in 2017.

Let's switch to the MSYS2-maintaned `mingw-w64-asciidoctor` instead,
so we don't have to maintain our own package anymore.

Ref: #118
Ref: git/git@55d2d81
Signed-off-by: Dennis Ameling <[email protected]>
dennisameling added a commit that referenced this pull request May 16, 2024
Ruby support was added to clangarm64 in November 2023,
so we can now enable asciidoctor on ARM64 as well.

Ref: msys2#19179
Ref: #118
Signed-off-by: Dennis Ameling <[email protected]>
@dennisameling
Copy link
Author

Ok, let's try: #119

@dennisameling that means the road is clear for a mingw-w64-asciidoctor-extensions-less future! Do note, though, that this package had its challenges in the past and the corresponding set of work-arounds (also this). Meaning: There might be a long tail of the project to drop that dependency ;-)

Thanks for the heads-up! I'd be more than happy to help with the long tail as well. Starting to get some more bandwidth again. Will file PR's for the two places you mentioned as well 👍🏼

dscho pushed a commit that referenced this pull request Jun 12, 2024
While enabling clangarm64 support on GfW's
`mingw-w64-asciidoctor-extensions` package, we realized that the
extensions are likely not needed anymore, because the Git project
added its own `Documentation/asciidoctor-extensions.rb` back in 2017.

Let's switch to the MSYS2-maintaned `mingw-w64-asciidoctor` instead,
so we don't have to maintain our own package anymore.

Ref: #118
Ref: git/git@55d2d81
Signed-off-by: Dennis Ameling <[email protected]>
dscho pushed a commit that referenced this pull request Jun 12, 2024
Ruby support was added to clangarm64 in November 2023,
so we can now enable asciidoctor on ARM64 as well.

Ref: msys2#19179
Ref: #118
Signed-off-by: Dennis Ameling <[email protected]>
@dscho
Copy link
Member

dscho commented Jun 12, 2024

@dennisameling let's go forward with #119 and close this here PR?

@dennisameling
Copy link
Author

@dennisameling let's go forward with #119 and close this here PR?

Indeed, thanks for helping out with the other PRs!

@dennisameling dennisameling deleted the asciidoctor-arm64 branch June 16, 2024 08:32
ammyk9 pushed a commit to ammyk9/MINGW-packages that referenced this pull request Aug 8, 2024
While enabling clangarm64 support on GfW's
`mingw-w64-asciidoctor-extensions` package, we realized that the
extensions are likely not needed anymore, because the Git project
added its own `Documentation/asciidoctor-extensions.rb` back in 2017.

Let's switch to the MSYS2-maintaned `mingw-w64-asciidoctor` instead,
so we don't have to maintain our own package anymore.

Ref: git-for-windows#118
Ref: git/git@55d2d81
Signed-off-by: Dennis Ameling <[email protected]>
ammyk9 pushed a commit to ammyk9/MINGW-packages that referenced this pull request Aug 8, 2024
Ruby support was added to clangarm64 in November 2023,
so we can now enable asciidoctor on ARM64 as well.

Ref: msys2#19179
Ref: git-for-windows#118
Signed-off-by: Dennis Ameling <[email protected]>
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.

3 participants