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

EJSTemplates: make typealiases accessible from templates #1208

Merged
merged 2 commits into from Oct 30, 2023
Merged

EJSTemplates: make typealiases accessible from templates #1208

merged 2 commits into from Oct 30, 2023

Conversation

djbe
Copy link
Contributor

@djbe djbe commented Oct 7, 2023

Currently marked with skipJSExport, so not visible from those templates. Not sure why 🤷

Same as with the recent protocols PR, added some tests for it as well.

@art-divin
Copy link
Collaborator

👋🏻 Thank you for your PR! It is very much appreciated 🤝

I am checking what is up with the CI. ⏲️

@art-divin
Copy link
Collaborator

👋🏻 Hey @djbe ,

I have tested your commit on Linux (same as on CI) and it worked for tests. Here on CI it traps with Signal 11.

So, I have updated Swift version to 5.9 in master branch (wonder when should we rename it to main 🤔 ).

Could you please rebase with the top of the branch and let's see how CI would react this time. Thank you 🤝

@djbe
Copy link
Contributor Author

djbe commented Oct 8, 2023

Sure thing 🤞

About master -> main, I think there are some guides about that transition. I know Github has some general info about it, but I'd check other resources. Especially impact on CI, maybe other integrations, …

@art-divin
Copy link
Collaborator

art-divin commented Oct 8, 2023

@djbe congratulations, you've uncovered yet another bug in Swift runtime on Linux 👍🏻

Jokes aside, I'll tinker with it. Wonder why locally cannot reproduce from your fork 🕵🏻

Edit: now I was able to reproduce, was testing a wrong branch 🙈
Edit2: crash is the same, apple/swift#68296 (comment), coming from SwiftTemplateSpec. Checking 👀

Edit3: crash happens also on master. This is really weird because CI was green for another PR I just merged. I'll take a look a bit later.

*** Program crashed: Bad pointer dereference at 0x00007fff053d6f98 ***
Thread 0 "SourceryPackage" crashed:
0      0x00007ff234205072 _swift_getGenericMetadata(swift::MetadataRequest, void const* const*, swift::TargetTypeContextDescriptor<swift::InProcess> const*) + 34 in libswiftCore.so
1 [ra] 0x00007ff2341578f9 _ConcreteHashableBox._downCastConditional<A>(into:) + 40 in libswiftCore.so
Thread 1:
0  0x00007ff232891117 <unknown> in libc.so.6
Thread 2:
0  0x00007ff23292601e <unknown> in libc.so.6
Registers:
rax 0x00000000ffffffff  4294967295
rdx 0x00007ff2343460d0  00 02 00 00 00 00 00 00 18 37 37 34 f2 7f 00 00  ·········774ò···
rcx 0x00007ff234452ff0  ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00  ÿÿÿÿ············
rbx 0x00007ff234371db4  d2 00 08 00 00 a2 ff ff ec ff ff ff 50 1b bf ff  Ò····¢ÿÿìÿÿÿP·¿ÿ
rsi 0x00007ff234371db4  d2 00 08 00 00 a2 ff ff ec ff ff ff 50 1b bf ff  Ò····¢ÿÿìÿÿÿP·¿ÿ
rdi 0x00007fff053d70d8  d0 60 34 34 f2 7f 00 00 d0 60 34 34 f2 7f 00 00  Ð`44ò···Ð`44ò···
rbp 0x00007fff053d70f0  50 71 3d 05 ff 7f 00 00 f9 78 15 34 f2 7f 00 00  Pq=·ÿ···ùx·4ò···
rsp 0x00007fff053d6fa0  140733281300384
r8 0x00007ff234371db4  d2 00 08 00 00 a2 ff ff ec ff ff ff 50 1b bf ff  Ò····¢ÿÿìÿÿÿP·¿ÿ
r9 0x00007ff234452ff0  ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00  ÿÿÿÿ············
r10 0x0000000000000001  1
r11 0x00007ff232a19ce0  20 74 e3 a3 30 56 00 00 a0 73 72 a3 30 56 00 00   tã£0V·· sr£0V··
r12 0x00007ff23434ce48  00 02 00 00 00 00 00 00 38 6c 37 34 f2 7f 00 00  ········8l74ò···
r13 0x0000000000000000  0
r14 0x00007ff23434ce48  00 02 00 00 00 00 00 00 38 6c 37 34 f2 7f 00 00  ········8l74ò···
r15 0x00007fff053d70d8  d0 60 34 34 f2 7f 00 00 d0 60 34 34 f2 7f 00 00  Ð`44ò···Ð`44ò···
rip 0x00007ff234205072  e8 89 f6 ff ff 49 89 c4 48 8b 50 58 48 89 44 24  è·öÿÿI·ÄH·PXH·D$
rflags 0x0000000000010206  PF
cs 0x0033  fs 0x0000  gs 0x0000
Images (57 omitted):
0x00007ff232800000–0x00007ff2329bc541 a43bfc8428df6623cd498c9c0caeb91aec9be4f9 libc.so.6       /usr/lib/x86_64-linux-gnu/libc.so.6
0x00007ff233e00000–0x00007ff23433dd68 <no build ID>                            libswiftCore.so /opt/hostedtoolcache/swift-5.9-RELEASE-ubuntu2204/5.9.0/x86_64/usr/lib/swift/linux/libswiftCore.so

@art-divin
Copy link
Collaborator

👋🏻 Hey @djbe ,

I am finally back from a short break, and I have investigated what happens and found the solution.

Here's what fails:

  1. Foo.swift file now contains typealias [:], which cannot be "parsed" by Linux version of Foundation's NSCoder (same bug as I have referenced before)
  2. The class name, "Bar", conflicts with a class name in file FileParserMethodsSpec used for tests

In order to overcome these two problems, the following actions need to be made:

  1. A separate Stub/Source folder needs to be created specifically for Linux, that would be a copy of the existing Stub/Source but with removed typealias in Foo.swift
  2. In Package.swift testTarget named SourceryLibTests needs to have resources conditionally compiled, with Stub/Source for canImport(ObjectiveC) case, and Stub/Source_Linux otherwise
  3. Class Bar needs to be renamed, say, to BarBaz
  4. Typealias.swift should also have the update related to the renamed FooBarBaz

Another solution is to have type instead of [String: String] set to [String] or some other than a dictionary type.

What do you think? I have pushed my fix, which is not 100% complete, here.

I hope this helps! Thank you 🤝

Copy link
Collaborator

@art-divin art-divin left a comment

Choose a reason for hiding this comment

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

Would be nice to address the Linux build, and I'd merge this PR immediately, thank you 👍🏻

@art-divin
Copy link
Collaborator

👋🏻 hey @djbe ,

would you have capacity to do the change, or I can take over? I'll need write access in your fork, or I'll copy your changes into a new PR.
What would you suggest?

Thank you 🙏🏻

@djbe
Copy link
Contributor Author

djbe commented Oct 30, 2023

Maybe some of the requested changes, but not all 😅

  • Completely restructuring the tests, just for that one line (typealias) because Swift itself is buggy, seems a bit overkill.
  • So I went with the simple solution of using a typealias to String.
  • I did apply the requested changes about renaming Foo & Bar (did both just in case, particularly because your commit renamed Foo).

Note: if you want feel free to push your own changes.

@art-divin
Copy link
Collaborator

Thank you @djbe ,

Linux tests still fail. What I think is the most reasonable to do is:

  1. Merge to enable the main branch to work with macOS
  2. Address Linux tests in a separate MR

This way we can unblock you and also can delegate the fix.

Thank you a lot 👍🏻

@art-divin art-divin merged commit 56661dc into krzysztofzablocki:master Oct 30, 2023
2 of 3 checks passed
@art-divin art-divin mentioned this pull request Oct 30, 2023
@djbe djbe deleted the feature/typealiases-in-ejs branch October 30, 2023 16:04
@djbe
Copy link
Contributor Author

djbe commented Oct 30, 2023

Hmmm that is unfortunate, the circle CI build was successful last night, but that probably didn't build on linux. Weird that it's still broken, because I got rid of the typealias with [String: String] (replaced it with a simpler one.

@art-divin
Copy link
Collaborator

No issue, I'll handle it since I have everything prepared. Thanks!

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

2 participants