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

Pack localized assemblies. #1816

Merged
merged 1 commit into from Jul 18, 2016

Conversation

Projects
None yet
3 participants
@JohanLarsson
Contributor

JohanLarsson commented Jul 17, 2016

Second attempt at #1806.

  • Should fix #1810.
  • Printing warning instead of throwing if satellite dll is not found. Added integration test for this.
  • Better pattern for finding languages.
  • Checking that matches for the regex is a culture name.

Thank you for all the help @smoothdeveloper!

Show outdated Hide outdated integrationtests/Paket.IntegrationTests/PackSpecs.fs
paket ("pack -v output \"" + outPath) scenario |> ignore
ZipFile.ExtractToDirectory(package, outPath)
Path.Combine(outPath, "lib", "net45", "LocalizedLib.dll") |> checkFileExists

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 17, 2016

Contributor

Could you use </> operator defined in Paket.IO (it is AutoOpen so you don't even have to open the namespace):

outPath </> "lib" </> "net45" </> "LocalizedLib.dll" |> checkFileExists

@smoothdeveloper

smoothdeveloper Jul 17, 2016

Contributor

Could you use </> operator defined in Paket.IO (it is AutoOpen so you don't even have to open the namespace):

outPath </> "lib" </> "net45" </> "LocalizedLib.dll" |> checkFileExists

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 17, 2016

Contributor

While it looks sweet I have not seen it in the rest of the code so not sure. Trying to stay consistent with the codebase. Also as a noob Path.Combine(...) is nice as I know what it does.
As always, thanks for taking the time to read!

@JohanLarsson

JohanLarsson Jul 17, 2016

Contributor

While it looks sweet I have not seen it in the rest of the code so not sure. Trying to stay consistent with the codebase. Also as a noob Path.Combine(...) is nice as I know what it does.
As always, thanks for taking the time to read!

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 17, 2016

Contributor

Yes, it's minor thing.

That same operator is defined in FAKE and pretty much any codebase dealing with paths so I thin it is not too far from reach for average F# developer.

You can use go to definition on operators also 😄

@smoothdeveloper

smoothdeveloper Jul 17, 2016

Contributor

Yes, it's minor thing.

That same operator is defined in FAKE and pretty much any codebase dealing with paths so I thin it is not too far from reach for average F# developer.

You can use go to definition on operators also 😄

@JohanLarsson JohanLarsson changed the title from Pack localized assemblies. WIP to Pack localized assemblies. Jul 18, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 18, 2016

Member

Why do we show warnings and when? I mean having no satellite assemblies is the 95% case

Member

forki commented Jul 18, 2016

Why do we show warnings and when? I mean having no satellite assemblies is the 95% case

@JohanLarsson

This comment has been minimized.

Show comment
Hide comment
@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

The warning is shown here, it happens when:

  1. A localization for language is found in the project file.
  2. No matching ../{language}/foo.resources.dll is found.
    In the previous version I threw here but it was too harsh I think.
    I can remove it of course but think it can be a good reminder to build before running paket pack.

No localization should not give a warning.

Contributor

JohanLarsson commented Jul 18, 2016

The warning is shown here, it happens when:

  1. A localization for language is found in the project file.
  2. No matching ../{language}/foo.resources.dll is found.
    In the previous version I threw here but it was too harsh I think.
    I can remove it of course but think it can be a good reminder to build before running paket pack.

No localization should not give a warning.

Pack localized assemblies take II.
Fixes since previous attempt:
- #1810 should be ok now.
- Outputting warning instead of throwing if satellite dll is not found.
- Better pattern for finding languages.
- Checking that matches for the regex is a culture name.
open System.Collections.Generic
open System.Globalization
[<CompilationRepresentation (CompilationRepresentationFlags.ModuleSuffix)>]

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

Unless you have a type Cultures under Paket namespace, I don't think we need those attributes (which is used to disambiguate between module and type bearing same name).

@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

Unless you have a type Cultures under Paket namespace, I don't think we need those attributes (which is used to disambiguate between module and type bearing same name).

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

I copy pasted them from the Cache module, no idea what they mean.

@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

I copy pasted them from the Cache module, no idea what they mean.

This comment has been minimized.

@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

This is needed for Cache module because there is also a Cache type under same namespace.

F# will coallesce both to expose them as Cache but the module need to be given that attribute which under the hood suffixes the static class with Module.

We don't need it for this Cultures since there is no Cultures type that would conflict.

@smoothdeveloper

smoothdeveloper Jul 18, 2016

Contributor

This is needed for Cache module because there is also a Cache type under same namespace.

F# will coallesce both to expose them as Cache but the module need to be given that attribute which under the hood suffixes the static class with Module.

We don't need it for this Cultures since there is no Cultures type that would conflict.

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

Thanks for the explanation, removing it.
Oh, too late.

@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

Thanks for the explanation, removing it.
Oh, too late.

This comment has been minimized.

@forki

forki Jul 18, 2016

Member

there is always room for new PR ;-)

@forki

forki Jul 18, 2016

Member

there is always room for new PR ;-)

This comment has been minimized.

@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

I was gonna try to fix the uri issue but was too slow :D

@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

I was gonna try to fix the uri issue but was too slow :D

This comment has been minimized.

@forki

forki Jul 18, 2016

Member

That one is a bit tricky. not sure if my fix was good. have to think about it a bit more.

@forki

forki Jul 18, 2016

Member

That one is a bit tricky. not sure if my fix was good. have to think about it a bit more.

@forki forki merged commit f9508c6 into fsprojects:master Jul 18, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 18, 2016

Member

there is still one related test failing on mono: https://travis-ci.org/fsprojects/Paket/builds/145555619

Member

forki commented Jul 18, 2016

there is still one related test failing on mono: https://travis-ci.org/fsprojects/Paket/builds/145555619

@JohanLarsson

This comment has been minimized.

Show comment
Hide comment
@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

Checking it out, didn't see that one before, strange.

Contributor

JohanLarsson commented Jul 18, 2016

Checking it out, didn't see that one before, strange.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 18, 2016

Member

I assume there is something not commited

Member

forki commented Jul 18, 2016

I assume there is something not commited

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jul 18, 2016

Member

#1816 pack localized happy path fails on my machine with:

System.Exception : Paket failed with:
Could not find 'paket.dependencies'. To use Paket with this solution, please run 'paket init' first.
If you have already run 'paket.init' then ensure that 'paket.dependencies' is located in the top level directory of your repository.
Like this:
MySourceDir
  .paket
  paket.dependencies
Member

forki commented Jul 18, 2016

#1816 pack localized happy path fails on my machine with:

System.Exception : Paket failed with:
Could not find 'paket.dependencies'. To use Paket with this solution, please run 'paket init' first.
If you have already run 'paket.init' then ensure that 'paket.dependencies' is located in the top level directory of your repository.
Like this:
MySourceDir
  .paket
  paket.dependencies
@JohanLarsson

This comment has been minimized.

Show comment
Hide comment
@JohanLarsson

JohanLarsson Jul 18, 2016

Contributor

Looks like the templates are missing, must have dumbed it during some rebase. Opening a new WIP PR for fixing it.

Contributor

JohanLarsson commented Jul 18, 2016

Looks like the templates are missing, must have dumbed it during some rebase. Opening a new WIP PR for fixing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment