fix: internal filesystem using path based on the underlying OS

[Monirzadeh]

Hi.
this PR try to fix #822

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (15b2a1e) 25.49% compared to head (92f9ef7) 25.49%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #823   +/-   ##
=======================================
  Coverage   25.49%   25.49%           
=======================================
  Files          47       47           
  Lines        5632     5632           
=======================================
  Hits         1436     1436           
  Misses       3999     3999           
  Partials      197      197           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Monirzadeh]

i test that on windows and linux and work fine to me.
please test that on macos too if it possible.
should we add a test for each platform separately?

[fmartingr]

i test that on windows and linux and work fine to me. please test that on macos too if it possible. should we add a test for each platform separately?

I think we could run the unit tests in all platforms in the CI. Can you run the unittests in windows? i believe there's already a unit test that checks that the assets.css file is returned properly, so that should fail without this change.

[fmartingr]

My assumption was correct, test is failing on windos. #824

Now there are several issues:

1. A lot of other tests are failing on windows 😂
2. I still don't know why filepath.Join is not working properly on windows.

[Monirzadeh]

My assumption was correct, test is failing on windos. #824

Now there are several issues:

1. A lot of other tests are failing on windows 😂

2. I still don't know why `filepath.Join` is not working properly on windows.

Welcome to windows world 😄
The bad thing is i should working on windows to fix bug And it is real nightmare.

Can we run test in windows with Ci?
Do you know is is it possible to simulate running test in widows (locally on my system)?

Update:
Good news.
I review some of the test that faild in windows most of them has same issue. Faild when they try read or write files.

[fmartingr]

Can we run test in windows with Ci? Do you know is is it possible to simulate running test in widows (locally on my system)?

I have no idea. You can try to get my CI changes in #824 and run the tests directly in Github, though it will be slow.

Update: Good news. I review some of the test that faild in windows most of them has same issue. Faild when they try read or write files.

Awesome! What was the issue?

[Monirzadeh]

Update: Good news. I review some of the test that faild in windows most of them has same issue. Faild when they try read or write files.

Awesome! What was the issue?

@fmartingr i do more investigation.
in general, most of the problem is because of afero (in different aspect).
specifically, #660 (that present in v1.6 rc5). if you switch to one commit before or v1.6 rc4 most of the test pass on windows (except one test in fronttest_test.go)

i try to read more about afero and some bug exist in afero in windows (i am not complete sure yet)
for example, we use absolute path to done recover functionality #683 in this line

shiori/internal/core/processing.go

Lines 146 to 156 in 84e5b09

	ebookPath := model.GetEbookPath(&book)
	req.Bookmark = book
	if strings.Contains(contentType, "application/pdf") {
	return book, false, errors.Wrap(err, "can't create ebook from pdf")
	} else {
	_, err = GenerateEbook(deps, req, ebookPath)
	if err != nil {
	return book, true, errors.Wrap(err, "failed to create ebook")
	}
	book.HasEbook = true

but afero can't handle absolute path correctly in windows. return error file does not exist that we use fs.MkdirAll in this line.

shiori/internal/domains/storage.go

Lines 75 to 79 in 84e5b09

	if dst != "" && !d.DirExists(dst) {
	err := d.fs.MkdirAll(filepath.Dir(dst), model.DataDirPerm)
	if err != nil {
	return fmt.Errorf("failed to create destination dir: %v", err)
	}

there are some bug reports similar issue (not exactly for this function) here spf13/afero#225
i am not sure should we fix this kind of bug in Shiori level or afero.
how should we handle that in Shiori without absolute path? do you have any idea? i try to avoid specific block code handle specific platform (make maintaining code harder and harder over the time)

I will continue to dive into other tests 😄

[fmartingr]

Hey @Monirzadeh thanks for the investigation. Honestly it may be easier to just switch afero to something different. We could lose some upgrades in the future but right now we are local-only so I'd rather not make you lose time with these kind of things. What do you think?

[Monirzadeh]

Do you know any alternative?
Maybe better to remove afero and merge new ci #824
Than we can start working to impeliment #805 with better test

[fmartingr]

Do you know any alternative?

Not at the moment, but we are just using local files, we should be able to create a basic implementation ourselves.