Skip to content

Commit

Permalink
Addressed PR comments.
Browse files Browse the repository at this point in the history
  • Loading branch information
taliesinb committed Nov 25, 2020
1 parent e10300e commit dfbbdbb
Show file tree
Hide file tree
Showing 20 changed files with 196 additions and 203 deletions.
26 changes: 18 additions & 8 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,9 +80,9 @@ Each change to the code must fundamentally pass through 5 steps, more or less in

### Building in-place

The main workflow we recommend is what we are calling an "in-place build". This largely happens automatically when you load the SetReplace package by calling `Get["~/git/SetReplace/Kernel/init.m"]` or equivalent. If the C++ library has not already been built, it will automatically be built for you, and the resulting libraries placed in the `LibraryResources` subdirectory of the repository root.
The main workflow we recommend is what we are calling an "in-place build". This largely happens automatically when you load the *SetReplace* package by calling `Get["~/git/SetReplace/Kernel/init.m"]` or equivalent. If the C++ library (*libSetReplace*) has not already been built, it will automatically be built for you, and the resulting libraries placed in the `LibraryResources` subdirectory of the repository root.

If you later modify the C++ code and call `Get[...]` again, the library will be automatically rebuilt, and hot-loaded into your current Mathematica session (you do not need to Quit). Moreover, builds of the library will be cached based on the hash of the C++ code, making it easy to switch quickly between several library versions (say, in different Git branches).
If you later modify the C++ code and call `Get[...]` again, the library will be automatically rebuilt, and hot-loaded into your current Mathematica session (you do not need to run `Quit[]`). Moreover, builds of the library will be cached based on the hash of the C++ code, making it easy to switch quickly between several library versions (say, in different Git branches).

If you wish to invoke the library build process *directly*, you run the following on the command line:

Expand All @@ -102,7 +102,7 @@ Doing so will automatically rebuild the library, if necessary.

### Build Paclets

You may occasionally want to build and install a paclet from the current state of the repository. This will package together all the Wolfram Language source code, along with the library, and various metadata, into a single ".paclet" file, which has a automatically computed version number associated with it.
You may occasionally want to build and install a paclet from the current state of the repository. This will package together all the Wolfram Language source code, along with the library, and various metadata, into a single ".paclet" file, which has an automatically computed version number associated with it.

To simply build the paclet, without installing it, just run:

Expand All @@ -120,9 +120,7 @@ cd ~/git/SetReplace
./install.wls
```

The paclet will be installed in your system, replacing any existing version of the paclet you may have. This will allow you to load the paclet in future by running simply ``Get["SetReplace`"] ``.

A less frequently updated version is available through the Wolfram public paclet server and can be installed with `PacletInstall["SetReplace"]`.
The paclet will be installed in your system, replacing any existing version of the paclet you may have. This will allow you to load the paclet in the future by running simply ``Get["SetReplace`"] ``.

### Writing code

Expand Down Expand Up @@ -150,7 +148,7 @@ It is essential to keep your pull requests as small as possible (definitely unde

### Automated tests

To run the tests, `cd` to the repository root, and run `./test.wls` from the command line. Note that `./test.wls` will automatically rebuild libSetReplace for you before running the tests if your libSetReplace is out of date (for example, if you changed some C++ files but you did not either `Get` SetReplace or run `./build.wls`). If everything is ok, you will see `[ok]` next to each group of tests, and "Tests passed." message at the end. Otherwise, you will see error messages telling you which test inputs failed and for what reason.
To run the tests, `cd` to the repository root, and run `./test.wls` from the command line. Note that `./test.wls` will automatically rebuild *libSetReplace* for you before running the tests if your *libSetReplace* is out of date (for example, if you changed some C++ files but you did not either `Get` *SetReplace* or run `./build.wls`). If everything is ok, you will see `[ok]` next to each group of tests, and "Tests passed." message at the end. Otherwise, you will see error messages telling you which test inputs failed and for what reason.

The `test.wls` script accepts various arguments. Running `./test.wls testfile`, where `testfile` is the name (without trailing `.wlt`) of a test file under the `Tests` directory, will limit the test run to only that file. With the `--load-installed-paclet` (or `-lip`) flag, the script will load the *installed paclet* instead of the local codebase. With the `--disable-parallelization` (or `-dp`) flag, you can disable the use of parallel sub-kernels to perform tests. Using parallel sub-kernels will accelerate running the entire test suite, so it is the default behavior, but if you are running only a single test file with a small number of tests, the startup time of the sub-kernels can outweight any speedup they give, and so `--disable-parallelization` will improve performance.

Expand Down Expand Up @@ -391,7 +389,19 @@ Some things to note are:

### Scripts

The three main scripts of *SetReplace* are [build.wls](/build.wls), [install.wls](/install.wls) and [test.wls](/test.wls). The build script is the most complex of the three, and it uses additional definitions in [buildInit.wl](/scripts/buildInit.wl). In addition to building the C++ code and packing the paclet, it also auto-generates the paclet version number based on the number of commits to master from the checkpoint defined in [version.wl](/scripts/version.wl). Some of the code in the [scripts](/scripts) folder is only used for building *SetReplace* on the internal Wolfram Research systems and should not be modified by external developers as CI has no way of testing it.
If you are using the *in-place* workflow, you will typically only need to run the [test.wls](/test.wls) script, but this section will describe the other scripts too.

The four main scripts of *SetReplace* are:
* [build.wls](/build.wls), which builds *libSetReplace* (if necessary)
* [test.wls](/test.wls), which first calls `build.wls`, then runs the full test suite.
* [pack.wls](/pack.wls), which first calls `build.wls`, then produces a `.paclet` file
* [install.wls](/install.wls), which first calls `pack.wls`, then installs the resulting `.paclet` file

All four scripts use functionality defined in the [DevUtils](/DevUtils) package.

Note that the `pack.wls` script will auto-generate the paclet version number based on the number of commits to master from the checkpoint defined in [version.wl](/scripts/version.wl).

The code in the [scripts](/scripts) folder is only used for building *SetReplace* on the internal Wolfram Research systems and should not be modified by external developers as CI has no way of testing it.

## Code style

Expand Down
10 changes: 3 additions & 7 deletions DevUtils/BuildLibrary.m → DevUtils/BuildLibSetReplace.m
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

PackageImport["GeneralUtilities`"]



PackageExport["BuildLibSetReplace"]

Options[BuildLibSetReplace] = {
Expand Down Expand Up @@ -47,7 +45,6 @@
BuildLibSetReplace::badsourcedir = "Source directory `` did not exist.";

BuildLibSetReplace[OptionsPattern[]] := ModuleScope[

(* options processing *)
UnpackOptions[
rootDirectory, librarySourceDirectory, libraryTargetDirectory,
Expand Down Expand Up @@ -78,8 +75,8 @@
calculateBuildData[] := Association[
"LibraryPath" -> libraryPath,
"LibraryFileName" -> libraryFileName,
"LibraryBuildTime" -> DateList[FileDate[libraryPath], TimeZone -> "UTC"],
"LibrarySourceHash" -> Hash[sourceHashes]
"LibraryBuildTime" -> Round[DateList[FileDate[libraryPath], TimeZone -> "UTC"]],
"LibrarySourceHash" -> finalHash
];

(* if a cached library exists with the right name, we can skip the compilation step, and need
Expand Down Expand Up @@ -167,10 +164,9 @@
Join[{"-std=c++17"}, $warningsFlags]
];


flushLibrariesIfFull[libraryDirectory_] := Scope[
files = FileNames["lib*", libraryDirectory];
If[Length[files] > 128,
If[Length[files] > 127,
oldestFile = MinimalBy[files, FileDate, 8];
Scan[DeleteFile, oldestFile]
]
Expand Down
27 changes: 16 additions & 11 deletions DevUtils/Console.m
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@

PackageImport["GeneralUtilities`"]


PackageExport["ConsoleBuildLibSetReplace"]

ConsoleBuildLibSetReplace[opts___] := ModuleScope @ Check[
SetUsage @ "
ConsoleBuildLibSetReplace[opts$$] calls BuildLibSetReplace[opts$$] but prints information to \
the command line when success or failure occurs. It also checks for messages and aborts if they \
occur. It returns a string of the library path on success, and $Failed on failure.
"

ConsoleBuildLibSetReplace[opts___] := ModuleScope @ Check[
Off[General::stop];

result = BuildLibSetReplace[opts, "PreBuildCallback" -> "Print"];
Expand All @@ -17,23 +21,27 @@
];

If[result["FromCache"],
Print["Using cached build"];
Print["Using cached build."];
];

Print["Library at ", result["LibraryPath"]];
Print["Build succeeded"];
Print["Build succeeded."];

result
,
Print["Message occurred during build. Build failed."];
$Failed
];


PackageExport["ConsoleCreateSetReplacePaclet"]

ConsoleCreateSetReplacePaclet[opts___] := ModuleScope @ Check[
SetUsage @ "
ConsoleCreateSetReplacePaclet[opts$$] calls CreateSetReplacePaclet[opts$$] but prints information to \
the command line when success or failure occurs. It also checks for messages and aborts if they \
occur. it returns a string of the paclet file path on success, and $Failed on failure.
"

ConsoleCreateSetReplacePaclet[opts___] := ModuleScope @ Check[
If[!AssociationQ[ConsoleBuildLibSetReplace[opts]],
Print["Build failed, creation of paclet aborted."];
ReturnFailed[]];
Expand All @@ -54,8 +62,6 @@
$Failed
];



PackageExport["ConsolePrintList"]

SetUsage @ "
Expand All @@ -69,12 +75,11 @@
Print["}"];
);



PackageExport["ConsoleTryEnvironment"]

SetUsage @ "
ConsoleTryEnvironment[var$, default$] will look up the value of the environment variable var$, but use default$ if it is not availabe.
ConsoleTryEnvironment[var$, default$] will look up the value of the environment variable var$, but use \
default$ if it is not availabe.
"

SetAttributes[ConsoleTryEnvironment, HoldRest];
Expand Down
27 changes: 12 additions & 15 deletions DevUtils/BuildPaclet.m → DevUtils/CreateSetReplacePaclet.m
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

PackageImport["GeneralUtilities`"]



PackageExport["CreateSetReplacePaclet"]

Options[CreateSetReplacePaclet] = {
Expand All @@ -12,23 +10,22 @@
"OutputDirectory" -> Automatic
};


CreateSetReplacePaclet::buildfailed = "Could not build paclet from `` into ``.";
CreateSetReplacePaclet::nogitlink = "GitLink is not installed, so the built paclet version cannot be correctly calculated. Proceed with caution, and consider installing GitLink by running InstallGitLink[]."
CreateSetReplacePaclet::packfailed = "Could not pack paclet from `` into ``.";
CreateSetReplacePaclet::nogitlink = "GitLink is not installed, so the built paclet version cannot be correctly \
calculated. Proceed with caution, and consider installing GitLink by running InstallGitLink[]."

SetUsage @ "
CreateSetReplacePaclet[] creates a PacletObject containing the local source and last built library.
* Note that CreateSetReplacePaclet[] does *not* call BuildLibSetReplace[], unlike the command line scripts.
* The PacletObject represents a .paclet file created on disk.
* The default location of the .paclet file is within the BuiltPaclets subdirectory of the current repo, \
but can be overriden with the 'OutputDirectory' option.
* The source for the PacletObject is given by the contents of the 'Kernel' and 'LibraryResources' directories \
of the current repo, but can be overridden with the 'RootDirectory' option.
* The minor version is derived from the number of commits since the last checkpoint, which is obtained from \
the 'master' branch, which can be overriden with the 'MasterBranch' option.
* The minor version is derived from the number of commits between the last checkpoint and the 'master' branch,
which can be overriden with the 'MasterBranch' option. The checkpoint is defined in `scripts/version.wl`.
"

$gitLinkDownloadCode = "PacletInstall[\"https://www.wolframcloud.com/obj/maxp1/GitLink-2019.11.26.01.paclet\"]";

CreateSetReplacePaclet[OptionsPattern[]] := ModuleScope[
UnpackOptions[rootDirectory, masterBranch, outputDirectory];
SetAutomatic[outputDirectory, FileNameJoin[{rootDirectory, "BuiltPaclets"}]];
Expand All @@ -44,26 +41,26 @@
];

buildInfo = <|"GitSHA" -> gitSHA, "BuildTime" -> Round[DateList[TimeZone -> "UTC"]]|>;
tempBuildInfoFile = FileNameJoin[{$TemporaryDirectory, "PacletBuildInfo.json"}];
tempBuildInfoFile = FileNameJoin[{$DevUtilsTemporaryDirectory, "PacletBuildInfo.json"}];
Developer`WriteRawJSONFile[tempBuildInfoFile, buildInfo];

fileTree = Flatten @ List[
fileTree = {
FileNameJoin[{rootDirectory, "Kernel"}],
FileNameJoin[{rootDirectory, "LibraryResources"}],
{pacletInfoFile, tempBuildInfoFile}
];
pacletInfoFile, tempBuildInfoFile
};
pacletFileName = CreatePacletArchive[fileTree, outputDirectory];
If[StringQ[pacletFileName],
Return[PacletObject[File[pacletFileName]]],
ReturnFailed["buildfailed", rootDirectory, outputDirectory]
ReturnFailed["packfailed", rootDirectory, outputDirectory]
]
];

createUpdatedPacletInfo[rootDirectory_, minorVersionNumber_] := ModuleScope[
pacletInfoFilename = FileNameJoin[{rootDirectory, "PacletInfo.m"}];
pacletInfo = Association @@ Import[pacletInfoFilename];
versionString = pacletInfo[Version] <> "." <> ToString[minorVersionNumber];
tempFilename = FileNameJoin[{$TemporaryDirectory, "PacletInfo.m"}];
tempFilename = FileNameJoin[{$DevUtilsTemporaryDirectory, "PacletInfo.m"}];
AppendTo[pacletInfo, Version -> versionString];
Block[{$ContextPath = {"System`", "PacletManager`"}},
Export[tempFilename, PacletManager`Paclet @@ Normal[pacletInfo]]
Expand Down
1 change: 0 additions & 1 deletion DevUtils/FileTreeHashes.m
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

PackageImport["GeneralUtilities`"]


PackageExport["FileTreeHashes"]

SetUsage @ "
Expand Down
11 changes: 7 additions & 4 deletions DevUtils/GitLink.m
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

PackageImport["GeneralUtilities`"]


PackageExport["$GitLinkAvailableQ"]

(* unfortunately, owing to a bug in GitLink, GitLink *needs* to be on the $ContextPath or GitRepo objects
Expand All @@ -28,7 +27,6 @@

GitSHAWithDirtyStar[rootDir_] /; FalseQ[$GitLinkAvailableQ] := Missing["NotAvailable"];


PackageExport["InstallGitLink"]

SetUsage @ "
Expand All @@ -42,16 +40,21 @@
];
];


PackageExport["CalculateMinorVersionNumber"]

SetUsage @ "
CalculateMinorVersionNumber[rootDirectory$, masterBranch$] will calculate a minor version \
derived from the number of commits between the last checkpoint and the 'master' branch, \
which can be overriden with the 'MasterBranch' option. The checkpoint is defined in scripts/version.wl.
"

CalculateMinorVersionNumber[rootDirectory_, masterBranch_] := ModuleScope[
versionInformation = Import[FileNameJoin[{rootDirectory, "scripts", "version.wl"}]];
gitRepo = GitLink`GitOpen[rootDirectory];
If[$internalBuildQ, GitLink`GitFetch[gitRepo, "origin"]];
minorVersionNumber = Max[0, Length[GitLink`GitRange[
gitRepo,
Except[versionInformation["Checkpoint"]],
GitLink`GitMergeBase[gitRepo, "HEAD", masterBranch]] - 1]];
GitLink`GitMergeBase[gitRepo, "HEAD", masterBranch]]] - 1];
minorVersionNumber
];
4 changes: 4 additions & 0 deletions DevUtils/init.m
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,12 @@
(* SetReplaceDevUtils is *not* included in paclet builds, so is not visible to users,
but is available for developer workflow purposes, and is used by the build scripts *)

PackageImport["GeneralUtilities`"]

PackageExport["$SetReplaceRoot"]
PackageExport["$DevUtilsRoot"]
PackageExport["$DevUtilsTemporaryDirectory"]

$SetReplaceRoot = FileNameDrop[$InputFileName, -2];
$DevUtilsRoot = FileNameDrop[$InputFileName, -1];
$DevUtilsTemporaryDirectory := EnsureDirectory @ FileNameJoin[{$TemporaryDirectory, "SetReplace"}];
14 changes: 8 additions & 6 deletions Kernel/buildData.m
Original file line number Diff line number Diff line change
Expand Up @@ -18,16 +18,18 @@

(* forwarders for the functions we want from DevUtils. This is done so
we don't create the SetReplaceDevUtils context for ordinary users (when DevUtils *isn't* available) *)
$buildLibSetReplace = Symbol["SetReplaceDevUtils`BuildLibSetReplace"];
$gitSHAWithDirtyStar = Symbol["SetReplaceDevUtils`GitSHAWithDirtyStar"];
buildLibSetReplace = Symbol["SetReplaceDevUtils`BuildLibSetReplace"];
gitSHAWithDirtyStar = Symbol["SetReplaceDevUtils`GitSHAWithDirtyStar"];

(* try build the C++ code immediately (which will most likely retrieve a cached library) *)
(* if there is a frontend, then give a temporary progress panel, otherwise just Print *)
If[TrueQ @ $Notebooks,
(* WithLocalSettings will run the final 'cleanup' argument even if the evaluation of the second
argument aborts (due to a Throw, user abort, etc.) *)
Internal`WithLocalSettings[
$progCell = None;
,
$buildResult = $buildLibSetReplace["PreBuildCallback" -> Function[
$buildResult = buildLibSetReplace["PreBuildCallback" -> Function[
$progCell = PrintTemporary @ Panel[
"Building libSetReplace from sources in " <> #LibrarySourceDirectory,
Background -> LightOrange]]];
Expand All @@ -36,7 +38,7 @@
$progCell = None;
];
,
$buildResult = $buildLibSetReplace["PreBuildCallback" -> "Print"];
$buildResult = buildLibSetReplace["PreBuildCallback" -> "Print"];
];

If[!AssociationQ[$buildResult],
Expand Down Expand Up @@ -76,7 +78,7 @@

SetUsage @ "
$SetReplaceBuildTime gives the time at which this SetReplace paclet was built.
* When evaluated for an in-place build, this time is Now.
* When evaluated for an in-place build, this time is the time at which SetReplace was loaded.
"

SetUsage @ "
Expand All @@ -90,7 +92,7 @@
$SetReplaceBuildTime = DateObject[$pacletBuildInfo["BuildTime"], TimeZone -> "UTC"];
$SetReplaceGitSHA = $pacletBuildInfo["GitSHA"];
,
$SetReplaceGitSHA = $gitSHAWithDirtyStar[$packageRoot];
$SetReplaceGitSHA = gitSHAWithDirtyStar[$packageRoot];
If[!StringQ[$SetReplaceGitSHA], Missing["GitLinkNotAvailable"]];
$SetReplaceBuildTime = DateObject[TimeZone -> "UTC"];
];
Loading

0 comments on commit dfbbdbb

Please sign in to comment.