USDAForestService / ForestVegetationSimulator

Forest Vegetation Simulation (FVS) - Growth and Yield Modeling software
Other
41 stars 26 forks source link

Update sourceList.txt files to match capitalization of NVEL files. #44

Open d-diaz opened 5 months ago

d-diaz commented 5 months ago

Working on Linux or MacOSX to build FVS from source, we can pull the source code including the NVEL submodule now using git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git and retrieve the NVEL code that was recently moved into the ForestVegetationSimulator/volume/NVEL folder as a submodule. However, many of the files within the NVEL repo are named with a mix of capitalizations while the FVS*_sourceList.txt files specify all these filenames to completely lower-cased.

Recommend updating the FVS*_sourceList.txt files to use the capitalization matching incoming NVEL repo rather then requiring the user to change all the filenames to lowercase to use existing sourceLists or to update sourceLists themselves.

Here is what currently happens (building using Windows Subsystem for Linux):

>> git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git
>> cd ForestVegetationSimulator/bin
>> make FVSpn
OS: ; OSTYPE: ; WIN: -Dunix  ; OSARCH: l64; SLIBSUFX: .so
PIC: -fPIC; PRGSUFX:
mingw64: ; FCprf: ; CCprf:
FVSprgs: FVSak FVSbc FVSbm FVSca FVSci FVScr FVScs FVSec FVSem FVSie FVSkt FVSls FVSnc FVSne FVSoc FVSon FVSop FVSpn FVSsn FVSso FVStt FVSut FVSwc FVSws
CANprgs: FVSbc FVSon
USprgs: FVSak FVSbm FVSca FVSci FVScr FVScs FVSec FVSem FVSie FVSkt FVSls FVSnc FVSne FVSoc FVSop FVSpn FVSsn FVSso FVStt FVSut FVSwc FVSws
make: *** No rule to make target '../volume/NVEL/beqinfo.inc', needed by 'FVSpn'.  Stop.

When all the files in the NVEL folder are renamed to be lower case, FVS builds successfully.

ghost commented 5 months ago

Did you rest that? I don't think it will work unless the file names in the include statements matche the names on the file system, exactly includine the case of each letter in the name This is the situation on systems with case sensitive file systems.

What really needs to happen, and is long overdue, is that the volume library needs to be changed so the the names exactly match.

On Mon, May 20, 2024, 10:25 PM wagnerds @.***> wrote:

We are aware of this issue and from our discussions with NVEL, it seems the "INCLUDE" statements within the NVEL files must also match the case of the actual filenames when in a Linux or MacOS environment. Currently, all the "INCLUDE" statements within NVEL are all listed as lowercase, so for the time being, our recommendation to those building in a Unix environment to use the following script in R to automatically convert the file naming case convention.

setwd("C:/code_repos/ForestVegetationSimulator/") fn = dir('./volume/NVEL/') fn = paste0('./volume/NVEL/',fn)

file.rename(fn, tolower(fn))

— Reply to this email directly, view it on GitHub https://github.com/USDAForestService/ForestVegetationSimulator/issues/44#issuecomment-2121150768, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVTTTN5VNEOFSNONL432D3ZDJL2ZAVCNFSM6AAAAABIAIK27WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMRRGE2TANZWHA . You are receiving this because you are subscribed to this thread.Message ID: <USDAForestService/ForestVegetationSimulator/issues/44/2121150768@ github.com>

d-diaz commented 5 months ago

After trying the renaming of a NVEL files within a single sourceList.txt file, I can confirm @nickcrookston was right, and the build fails with the same error, first triggered by BEQINFO.inc

Agree that fixing this upstream would be ideal. That being said, I'd trust y'all's better judgement whether any of you could submit a pull request to NVEL to get that moving or if NVEL would accept a pull request from someone like me. Perhaps in the meantime y'all could make a fork of the NVEL library, update the file names there, and then have that version of the NVEL equations employed in this repo instead of using the main NVEL repo as a submodule? For example, maybe you could update the FMSC-Measurements/VolumeLibrary repo?

wagnerds commented 5 months ago

I'll have another discussion with NVEL. Last time I brought it up, they do not accept pull requests and prefer to do things via email.

jgrn307 commented 4 months ago

Is there a suggested "hack" fix in the meantime? We have an automatic build system and this broke our workflow.

jgrn307 commented 4 months ago

Update: what's even weirder is I've been rolling back the SHA I'm using to try to build off an earlier version of this (we were able to do the build a few months back) and I can't get it to build no matter what SHA I use (e.g. I'm back to using January 2024 commit but that's giving me the same error). Is this connected to a different github repo or something for those files? I don't understand why rolling back my pull isn't working to get this functional again.

wagnerds commented 4 months ago

@jgrn307 What was the source of the SHA you are reverting to? Was is just based off the date listed in this repository or from a source you previously had working? I suspect the SHA you may be referencing from January was the state of our code base in our Enterprise repository from that date, where the submodule repository change was already in effect. We held off on releasing that update until now as we only have Windows machines to test on.

Are you able to redirect your build to a previous tagged release? Maybe https://github.com/USDAForestService/ForestVegetationSimulator/tree/FS2023.3 ?

Also, what is your build environment? Some version of Linux I assume?

wagnerds commented 4 months ago

@jgrn307 @d-diaz

I may have misunderstood a previous comment. @d-diaz You said it built successfully when you changed the NVEL files to lowercase. I had posted an R script that should help users convert their local copy of the NVEL files to all lowercase, and may have misunderstood the response.

You said you renamed the files in the FVSxx_SourceList.txt to match the incoming NVEL files, and that didn't work as the file names did not match the NVEL 'INCLUDE' statements, which makes sense, but did you try the script I included that would convert the NVEL files to lowercase to match the 'INCLUDE' statements as well as the sourceList.txt file names?

Can you help me confirm whether running this script in R changes the file names within the volume/NVEL/ directory to lowercase within you local copy of NVEL? setwd("PathToRepository/ForestVegetationSimulator/") fn = dir('./volume/NVEL/') fn = paste0('./volume/NVEL/',fn) file.rename(fn, tolower(fn))

This seems to work in my instance of WSL, so I am looking for a bit of feedback on if this is a viable workaround.

jgrn307 commented 4 months ago

I was able to get the build working by reverting to https://github.com/USDAForestService/ForestVegetationSimulator/commit/3b09ae2643866ec88149dabb91ae6b1bd5626f86 without the aforementioned error. I'm doing a linux-based Docker build of FVS.

I'll try the other tag tomorrow when I get back to this!

--j

@jgrn307 What was the source of the SHA you are reverting to? Was is just based off the date listed in this repository or from a source you previously had working? I suspect the SHA you may be referencing from January was the state of our code base in our Enterprise repository from that date, where the submodule repository change was already in effect. We held off on releasing that update until now as we only have Windows machines to test on.

Are you able to redirect your build to a previous tagged release? Maybe https://github.com/USDAForestService/ForestVegetationSimulator/tree/FS2023.3 ?

Also, what is your build environment? Some version of Linux I assume?

d-diaz commented 4 months ago

@jgrn307 @d-diaz

I may have misunderstood a previous comment. @d-diaz You said it built successfully when you changed the NVEL files to lowercase. I had posted an R script that should help users convert their local copy of the NVEL files to all lowercase, and may have misunderstood the response.

You said you renamed the files in the FVSxx_SourceList.txt to match the incoming NVEL files, and that didn't work as the file names did not match the NVEL 'INCLUDE' statements, which makes sense, but did you try the script I included that would convert the NVEL files to lowercase to match the 'INCLUDE' statements as well as the sourceList.txt file names?

Can you help me confirm whether running this script in R changes the file names within the volume/NVEL/ directory to lowercase within you local copy of NVEL? setwd("PathToRepository/ForestVegetationSimulator/") fn = dir('./volume/NVEL/') fn = paste0('./volume/NVEL/',fn) file.rename(fn, tolower(fn))

This seems to work in my instance of WSL, so I am looking for a bit of feedback on if this is a viable workaround.

I implemented a lower casing step in a shell script and was able to resume builds for US modules. Because the source lists for Canadian variants haven’t been updated to point to new NVEL paths, they are still failing:

cd ~/projects 
git clone --recurse-submodules https://github.com/USDAForestService/ForestVegetationSimulator.git
cd ~/projects/ForestVegetationSimulator/volume/NVEL
for f in `find .`; do mv -v "$f" "`echo $f | tr '[A-Z]' '[a-z]'`"; done
cd ~/projects/ForestVegetationSimulator/bin
make US -j12
sudo find ~/projects/ForestVegetationSimulator/bin/FVS* -maxdepth 0 -type f ! -name "*_sourceList.txt" -exec cp -t /usr/local/bin {} +
make clean

This is a duct-tape fix. Ideally FVS source code will be provided in a way that doesn’t require these kinds of patches by the user to build.

Even if y’all are working on windows, would encourage using WSL or Docker to run build tests. As I mentioned above, to fix this without waiting for NVEL, you could incorporate a different repo as the submodule, such as a fork of the NVEL repo where y’all have already lower-cased the file names.

d-diaz commented 2 weeks ago

Any update on potential upstream fixes to build error due to capitalizations within volume/NVEL? If NVEL isn't likely to address these capitalization issues at source, an FVS fork of the NVEL repo with capitalization fixed so that you could incorporate the forked version as a submodule would be a relatively lightweight fix.