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

181 paratext data linux support #216

Merged
merged 12 commits into from
Jun 21, 2023
Merged

Conversation

FoolRunning
Copy link
Contributor

@FoolRunning FoolRunning commented Jun 5, 2023

Just a draft PR to see if the C# tests testing loading ParatextData will run on Linux/Mac


This change is Reviewable

@lyonsil lyonsil force-pushed the 181-paratext-data-linux-support branch 4 times, most recently from 9742c06 to 8dd4a71 Compare June 19, 2023 21:56
@lyonsil lyonsil marked this pull request as ready for review June 19, 2023 22:23
Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

This has been opened up to a "real" PR because I believe it's ready for feedback now that it is working. We might choose to reject it if there is still substantial work to finish first, but this is important enough to try to move forward.

Reviewable status: 0 of 96 files reviewed, all discussions resolved

@lyonsil lyonsil linked an issue Jun 19, 2023 that may be closed by this pull request
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

I know this has been a lot of work to get it this far. Thanks for all the work on this from both of you.

Reviewed 91 of 93 files at r1, 3 of 5 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @FoolRunning)


assets/ATTRIBUTION.md line 1 at r3 (raw file):

# Attribution

I don't think you want these here as this is a specific folder used by electron to deliver items to the renderer. If you do want them in the renderer then you might need to update assests.d.ts. Otherwise move all these added assets.


assets/usfm.sty line 1 at r3 (raw file):

# Version=3.0.4

This is an almost identical file to the one in the WEB folder. Are they meant to be the same? Do we need both? Either way only one is referenced in the assets/ATTRIBUTION.md file.


c-sharp/Program.cs line 78 at r3 (raw file):

    #region DummyRegistry class
    private sealed class DummyRegistry : RegistryU

BTW is this a temporary solution to get things going and will eventually be removed? If not we probably don't want this defined in this file.


c-sharp-tests/ParatextDataConnectionTests.cs line 24 at r3 (raw file):

            if (!File.Exists(icuConfigFile))
            {
                // It's a bit hacky to hard code the config file here, but there isn't a great

Actually, its kinda nice to see what config is being used. It might even be useful to control it for given tests in the future.

@irahopkinson
Copy link
Contributor

BTW when I run this branch I get these errors in the terminal:
image.png
Presummably that's because I don't have the right ICU installed? If so we should add instructions to the README about installing it and think about how we would deliver it on a production build on all OSes.

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

@irahopkinson That particular error is very likely caused by a missing icu.net.dll.config file. When running normally (i.e., npm start or from VSCode), the config file should have been put there by dotnet restore as it is part of the dependency package. When running from npm test, there is a known NUnit bug that causes the file not to copy there, but a workaround is checked into the code.

It would be good to understand why that config file is missing in your case. If this comes up regularly, we might need to put the NUnit workaround in our normal code path so we always inject the file if it is missing.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @FoolRunning and @irahopkinson)


assets/ATTRIBUTION.md line 1 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

I don't think you want these here as this is a specific folder used by electron to deliver items to the renderer. If you do want them in the renderer then you might need to update assests.d.ts. Otherwise move all these added assets.

Sure - Do you have a suggested location name?


assets/usfm.sty line 1 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

This is an almost identical file to the one in the WEB folder. Are they meant to be the same? Do we need both? Either way only one is referenced in the assets/ATTRIBUTION.md file.

This is likely a mistake. @FoolRunning Do we need usfm.sty in the parent directory of WEB and in WEB itself?


c-sharp/Program.cs line 78 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW is this a temporary solution to get things going and will eventually be removed? If not we probably don't want this defined in this file.

I think this is a permanent solution, but @FoolRunning should confirm.

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

@irahopkinson Actually can you run "Debug .NET Core" in VSCode and see what the output shows? That will give more details on ICU loading errors. It could be a few different things.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @FoolRunning and @irahopkinson)

Copy link
Contributor Author

@FoolRunning FoolRunning left a comment

Choose a reason for hiding this comment

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

Since I'm the original creator of the PR, I can't block it, but consider it blocked until the things I mentioned are cleared up. 😄

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @irahopkinson and @lyonsil)


assets/usfm.sty line 1 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

This is likely a mistake. @FoolRunning Do we need usfm.sty in the parent directory of WEB and in WEB itself?

The one in the parent directory (this one) is the one that should be kept. The one in the WEB directory should be deleted.


assets/WEB/en.xml line 1 at r3 (raw file):

<?xml version="1.0" encoding="utf-8"?>

I'm fairly certain this file should be deleted. This is actually an LDML file with an XML extension.


c-sharp/Program.cs line 78 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I think this is a permanent solution, but @FoolRunning should confirm.

Yes, it is permanent (at least through the tech demo). Eventually we might want to get the information for the real Paratext install from the Windows Registry, but I don't think we want to do that yet since it could be potentially dangerous (corruption of a project, for example).

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 93 of 96 files reviewed, 2 unresolved discussions (waiting on @irahopkinson)


assets/WEB/en.xml line 1 at r3 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

I'm fairly certain this file should be deleted. This is actually an LDML file with an XML extension.

Done

Copy link
Contributor Author

@FoolRunning FoolRunning left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 93 of 96 files reviewed, 2 unresolved discussions (waiting on @irahopkinson)

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

Reviewable status: 4 of 96 files reviewed, 2 unresolved discussions (waiting on @irahopkinson)


assets/ATTRIBUTION.md line 1 at r3 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

Sure - Do you have a suggested location name?

Moved the relevant files in the assets folder under c-sharp/assets per our discussion


assets/usfm.sty line 1 at r3 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

The one in the parent directory (this one) is the one that should be kept. The one in the WEB directory should be deleted.

Done

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

I'm still getting this error on Windows.

Reviewed 2 of 3 files at r4, 89 of 89 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FoolRunning)


assets/ATTRIBUTION.md line 1 at r1 (raw file):

# Attribution

BTW we don't need to shout with the filename, i.e. ALLCAPS. Just use lowercase. We do shout for README.md to get peoples attention to start there.


assets/usfm.sty line 1 at r3 (raw file):

Previously, FoolRunning (Tim Steenwyk) wrote…

The one in the parent directory (this one) is the one that should be kept. The one in the WEB directory should be deleted.

Do note the one in the WEB folder is a later version so should probaly overwrite this one.

Copy link
Member

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

I discovered that Microsoft built ICUUC DLLs and makes them available as project references. I just added it to our c-sharp project, and it got past the error on my laptop.

Reviewable status: 94 of 96 files reviewed, 1 unresolved discussion (waiting on @irahopkinson)


assets/ATTRIBUTION.md line 1 at r1 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

BTW we don't need to shout with the filename, i.e. ALLCAPS. Just use lowercase. We do shout for README.md to get peoples attention to start there.

I can change it, but we do it for all of our .md files, include README, CODE_OF_CONDUCT, and CHANGELOG. We actually don't have a change that uses lowercase letters. This can be our first if we want to change that approach.


assets/usfm.sty line 1 at r3 (raw file):

Previously, irahopkinson (Ira Hopkinson) wrote…

Do note the one in the WEB folder is a later version so should probaly overwrite this one.

Ah, good point! Updated to the newer version (which matches the version currently at the URL in the attribution).

@lyonsil lyonsil force-pushed the 181-paratext-data-linux-support branch from 0c710ca to b6a5cb0 Compare June 20, 2023 23:12
irahopkinson
irahopkinson previously approved these changes Jun 20, 2023
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

Good find.
:lgtm: It's now working without throwing an exception in Windows. I can see the time ticking again.

Reviewed 1 of 1 files at r7, 1 of 1 files at r8, 1 of 5 files at r10, 93 of 94 files at r11, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @FoolRunning)


assets/ATTRIBUTION.md line 1 at r1 (raw file):

Previously, lyonsil (Matt Lyons) wrote…

I can change it, but we do it for all of our .md files, include README, CODE_OF_CONDUCT, and CHANGELOG. We actually don't have a change that uses lowercase letters. This can be our first if we want to change that approach.

Yes please since it's new. We inherited those other three from ERB.

Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r12, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @FoolRunning)

@lyonsil lyonsil merged commit f1bd945 into main Jun 21, 2023
@lyonsil lyonsil deleted the 181-paratext-data-linux-support branch June 21, 2023 00:55
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.

Load project data from non-Paratext 9 folder
3 participants