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

Split ARCtrl project into language-specific project files #446

Merged
merged 12 commits into from
Oct 7, 2024
Merged

Conversation

HLWeil
Copy link
Member

@HLWeil HLWeil commented Oct 2, 2024

Removed all python and javascript specific dependencies from all projects except for top-level ARCtrl.

There, created an ARCtrl.Javascript and ARCtrl.Python project file, which contain the corresponding dependencies and IO-implementations.

closes #445

@MangelMaxime

Comment on lines 4 to 65
<TargetFramework>netstandard2.0</TargetFramework>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<DefineConstants>FABLE_COMPILER_JAVASCRIPT;FABLE_COMPILER;FABLE_COMPILER_TYPESCRIPT</DefineConstants>
</PropertyGroup>
<PropertyGroup>
<Authors>nfdi4plants, Lukas Weil, Kevin Frey, Kevin Schneider, Oliver Maus</Authors>
<Description>Library for management of Annotated Research Contexts (ARCs) using an in-memory representation and runtimer agnostic contract systems.</Description>
<PackageLicenseExpression>MIT</PackageLicenseExpression>
<PackageIcon>logo.png</PackageIcon>
<PackageTags>ARC F# FSharp dotnet .Net bioinformatics biology fable-library datascience dataplant nfdi metadata</PackageTags>
<PackageProjectUrl>https://github.com/nfdi4plants/ARCtrl</PackageProjectUrl>
<RepositoryUrl>https://github.com/nfdi4plants/ARCtrl</RepositoryUrl>
<RepositoryType>git</RepositoryType>
</PropertyGroup>
<ItemGroup>
<Compile Include="Json\Decode.fs" />
<Compile Include="Json\Encode.fs" />
<Compile Include="Json\Comment.fs" />
<Compile Include="Json\OntologyAnnotation.fs" />
<Compile Include="Json\OntologySourceReference.fs" />
<Compile Include="Json\DataFile.fs" />
<Compile Include="Json\Data.fs" />
<Compile Include="Json\Person.fs" />
<Compile Include="Json\Publication.fs" />
<Compile Include="Json\Process\Value.fs" />
<Compile Include="Json\Process\Factor.fs" />
<Compile Include="Json\Process\FactorValue.fs" />
<Compile Include="Json\Process\ProtocolParameter.fs" />
<Compile Include="Json\Process\MaterialType.fs" />
<Compile Include="Json\Process\MaterialAttribute.fs" />
<Compile Include="Json\Process\Component.fs" />
<Compile Include="Json\Process\Protocol.fs" />
<Compile Include="Json\Process\MaterialAttributeValue.fs" />
<Compile Include="Json\Process\Material.fs" />
<Compile Include="Json\Process\Source.fs" />
<Compile Include="Json\Process\Sample.fs" />
<Compile Include="Json\Process\ProcessParameterValue.fs" />
<Compile Include="Json\Process\ProcessInput.fs" />
<Compile Include="Json\Process\ProcessOutput.fs" />
<Compile Include="Json\Process\Process.fs" />
<Compile Include="Json\Process\ProcessSequence.fs" />
<Compile Include="Json\Process\StudyMaterials.fs" />
<Compile Include="Json\Table\Compression.fs" />
<Compile Include="Json\Table\CompositeCell.fs" />
<Compile Include="Json\Table\IOType.fs" />
<Compile Include="Json\Table\CompositeHeader.fs" />
<Compile Include="Json\Table\ArcTable.fs" />
<Compile Include="Json\Table\Templates.fs" />
<Compile Include="Json\DataMap\DataMap.fs" />
<Compile Include="Json\Assay.fs" />
<Compile Include="Json\Study.fs" />
<Compile Include="Json\Investigation.fs" />
<Compile Include="Json\ROCrateObject.fs" />
<Compile Include="Json\ARC.fs" />
<Compile Include="WebRequest\WebRequest.Node.fs" />
<Compile Include="WebRequest\WebRequest.fs" />
<Compile Include="Template.Web.fs" />
<Compile Include="ARC.fs" />
<Compile Include="Json.fs" />
<Compile Include="Xlsx.fs" />
<None Include="README.md" />
</ItemGroup>

Choose a reason for hiding this comment

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

If you want to avoid duplication of the code you can move it to ARCTrl.Common.props and include it in your different project.

Some of the properties (like Authors, License) can even be moved to the top level Directory.Build.props

Copy link
Member Author

Choose a reason for hiding this comment

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

Will try, thanks!

<ProjectReference Include="..\Spreadsheet\ARCtrl.Spreadsheet.fsproj" />
</ItemGroup>
<ItemGroup>
<Content Include="*.fsproj; **\*.fs; **\*.fsi" PackagePath="fable\" />

Choose a reason for hiding this comment

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

Using Fable.Package.SDK allows to remove these lines and will asked you to set the PackageTags information so your NuGet package can be listed in https://fable.io/packages/

Copy link
Member Author

Choose a reason for hiding this comment

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

This did not work for me out of the box, will open an Issue in the given project.

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/fslaborg/FsSpreadsheet/blob/e59da935ebced63c3bcd7ed9e9dd3e276715fe54/src/FsSpreadsheet/FsSpreadsheet.fsproj#L6

Here are the lines in question. I thought this would be enough to trigger the inclusion of the src files in a Fable folders when running dotnet pack

Choose a reason for hiding this comment

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

It should be enough yes,

Use the following to include the dependency, you didn't have it included by default because it is only done automatically since 1.1.0. This will make your package not include Fable.Package.SDK in its dependencies.

<PackageReference Include="Fable.Package.SDK">
  <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
  <PrivateAssets>all</PrivateAssets>
</PackageReference>

You should also remove:

<Content Include="*.fsproj; **\*.fs; **\*.fsi" PackagePath="fable\" />

If this is still not working I can have a look at it if you want

Copy link
Member Author

@HLWeil HLWeil Oct 2, 2024

Choose a reason for hiding this comment

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

Hey, sry forgot to write here. The Inclusion of the Source Files works now! (I saw you released the 1.1.0 version)

Just got another small issue: fable-compiler/Fable.Package.SDK#6

It does produce a proper nuget file. So ignoring the error kinda works, but would be nice to find the reason behind this.

Choose a reason for hiding this comment

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

Its strange in theory if there is an error it should fails.

But perhaps MSBuild create the package before and then the CLI exist with an error code. I will check the issue you reported.

Choose a reason for hiding this comment

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

The issue is caused by this line:

<PackageTags>ARC F# FSharp dotnet .Net bioinformatics biology fable-library datascience dataplant nfdi metadata</PackageTags>

Judging by the content of the tag, I think you wanted to use <Description>

Copy link
Member Author

Choose a reason for hiding this comment

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

Its strange in theory if there is an error it should fails.

But perhaps MSBuild create the package before and then the CLI exist with an error code. I will check the issue you reported.

Yeah sry with "ignoring the error" I meant actually catching and ignoring the error :D

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue is caused by this line:

<PackageTags>ARC F# FSharp dotnet .Net bioinformatics biology fable-library datascience dataplant nfdi metadata</PackageTags>

Judging by the content of the tag, I think you wanted to use <Description>

image

These are actually tags used by nuget to increase findability of the package.

Copy link
Member Author

@HLWeil HLWeil Oct 2, 2024

Choose a reason for hiding this comment

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

Kinda hotfixed it by merging the two lists (See issue on Fable.Packages.SDK)

@HLWeil
Copy link
Member Author

HLWeil commented Oct 2, 2024

As far as I can tell everything works. I just pushed version 2.1.0-alpha1 to Nuget, npm and pypi.

@HLWeil
Copy link
Member Author

HLWeil commented Oct 2, 2024

So.. It's ready to review @Freymaurer @kMutagene

Thanks a lot for your input, @MangelMaxime!

@@ -163,6 +163,7 @@ let build = BuildTask.create "Build" [clean] {
let msBuildParams =
{p.MSBuildParams with
DisableInternalBinLog = true
NodeReuse = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this added?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, removed it

@@ -104,7 +104,7 @@ module BundlePy =
run python "-m poetry build" ProjectInfo.pyPkgDir //Remove "-o ." because not compatible with publish


let packPy = BuildTask.create "PackPy" [clean; build; runTests] {
let packPy = BuildTask.create "PackPy" [clean; build; (*runTests*)] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💀 commented out tests(!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, fixed

package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

i am not really a fun of reflection in f# code. I fully trust you in this, that we do not have to rework it in 2 years

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it's always a bit dangerous, but here I think we should be fine, as the Option part can be ignored in js and py.

Copy link
Collaborator

Choose a reason for hiding this comment

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

🚀❤️

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.

[Feature Request] Separation of ARCtrl into dotnet, js and python
3 participants